Skip to content

Conversation

@jkmassel
Copy link
Contributor

@jkmassel jkmassel commented May 27, 2024

In #22296, we merged a very large PDF file as an xcassets resource.

This increased the binary size by 43.1MB, causing it to be too large for installation or update on cellular networks.

This PR resolves that by:

  1. Switching out the PDF file for a JPEG-compressed gradient. If I'd had more time, I would've probably generated the gradient in code instead of as a static image, but that can be left for another day.
  2. Running perceptually invisible compression on all PNG assets (which saves another 0.5 MB)
Screenshot 2024-05-27 at 3 18 52 PM

To test:

Regression Notes

  1. Potential unintended areas of impact
  • Modifies many icon images – use the left/right comparison on GitHub to note that they're visually indistinguishable.
  • Run the first-launch process, note that there's no user-visible difference.
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • I did the items above.
  1. What automated tests I added (or what prevented me from doing so)
  • No automated tests available for this.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@dangermattic
Copy link
Collaborator

dangermattic commented May 27, 2024

1 Warning
⚠️ This PR is assigned to the milestone 25.0 ❄️. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

@jkmassel jkmassel force-pushed the reduce/application-binary-size branch from ae591bf to 3df21d1 Compare May 27, 2024 21:09
@jkmassel jkmassel marked this pull request as ready for review May 27, 2024 21:15
@jkmassel jkmassel self-assigned this May 27, 2024
@jkmassel jkmassel requested review from alpavanoglu and guarani May 27, 2024 21:15
@jkmassel jkmassel added this to the 25.0 ❄️ milestone May 27, 2024
@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 27, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23261-aca7b63
Version25.0
Bundle IDorg.wordpress.alpha
Commitaca7b63
App Center BuildWPiOS - One-Offs #10011
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 27, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23261-aca7b63
Version25.0
Bundle IDcom.jetpack.alpha
Commitaca7b63
App Center Buildjetpack-installable-builds #9062
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jkmassel!

I realized the Stats PDF isn't used, so it can be removed.

I tested the other change to the welcome screen and noticed it looks odd in dark mode:

this branch trunk

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkmassel, this resource is unused and can be removed entirely.
We removed the screen that used it in #19917, but the PDF was overlooked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Addressed in ac03070

@alpavanoglu
Copy link
Contributor

alpavanoglu commented May 28, 2024

Hey @jkmassel

This increased the binary size by 43.1MB

So that I have a better understanding and avoid this kind of thing in the future, how does that pdf file increase the binary size by 43.1 Megabytes when the file itself appear to be 5 Kilobytes? I understand the vector file will be converted but still the number is extremely high indeed.

If you mean the gradient, shouldn't the trade there lower the size rather than increase it?

Screenshot 2024-05-28 at 16 21 49

jkmassel added 2 commits May 28, 2024 19:11
Xcode’s SVG handling is interesting – it doesn’t properly handle filters, so the blur isn’t baked into the resulting image.

This is actually a feature, not a bug – the old encoding approach was worst-case scenario for image size – a partially-transparent image with gradients (from the blur) and noise applied overtop; and because Xcode turns the SVG into a PNG for us it would get unexpected large very quickly.

Doing the blur manually takes just as much CPU time as it would if we were processing the SVG within iOS, but results in a much smaller image inside the bundle.
@jkmassel
Copy link
Contributor Author

jkmassel commented May 29, 2024

@alpavanoglu – I'm not sure I understand?

how does that pdf file increase the binary size by 43.1 Megabytes when the file itself appear to be 5 Kilobytes

Where did you get 5kb from? The binary size is increased so much because the 7 MB PDF file is converted to a 3x, 2x, and 1x PNG by Xcode during compilation. The resulting Assets.car file is duplicated in the App Binary – it's also part of WordPressShareExtension, WordPressDraftActionExtension, andJetpackIntents, so the cost has to be paid four times.

If you mean the gradient, shouldn't the trade there lower the size rather than increase it?

Interesting – I suspected #22296 because removing this file reduced the binary size by the 43 MB and we got a notification from Apple on Feb 20 that the app had exceeded the download limit. Perhaps there is another factor at play?

@jkmassel
Copy link
Contributor Author

Thanks @guarani – the transparency adds a bunch of complexity – the issue should now be resolved. Unfortunately the savings aren't quite as dramatic as with a JPEG but it's a good start. Brings down the Jetpack binary size from 269.8MB to 210.6MB, so we're much closer to the cellular limit now!

@jkmassel jkmassel requested a review from guarani May 29, 2024 01:57
@alpavanoglu
Copy link
Contributor

alpavanoglu commented May 29, 2024

@jkmassel

Where did you get 5kb from?

That PR added 1 PDF and replaced the other. The pure addition is the small one I referred to there. And the replaced PDF is smaller in size so I reckoned overall I would be reducing the size and not increase it.

Perhaps there is another factor at play?

Perhaps. I wanted to clarify to avoid such things in the future.

@jkmassel
Copy link
Contributor Author

Addressed in #23381

@jkmassel jkmassel closed this Jun 24, 2024
@jkmassel jkmassel deleted the reduce/application-binary-size branch June 24, 2024 20:19
@kean kean mentioned this pull request Jun 28, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants