Re: Android binary size increase on refactor CLs

[Andrew Grieve]

Yikes, I changed that mailing-list's permissions after it started getting a lot of spam, and accidentally restricted who can join :/. It should be the case now that anyone can join, and all members can post.

It's definitely a bug that diagnose_bloat.py requires that branding file. I'll look into fixing that.

The reason the stacked change is hitting the alert, is that the bot includes dependent changes, so it's really just the first change that's triggering it.

My initial guess is that it's related to PGO profiles, and will likely go away once the profiles get regenerated.  We have https://crbug.com/344608183 about disabling PGO for the size bot, but haven't gotten to it yet.

Given that the arm32 size is fine, feel free to appease the bot by adding a "Binary-Size" footer, and we can follow up once diagnose_bloat.py is fixed to confirm if this is PGO or not.

On Thu, Jun 27, 2024 at 11:27 AM Kevin Babbitt <kbab...@microsoft.com> wrote:

Hi Andrew,

 

I’m reaching out to you because my message to binary-size@ got a “Delivery Status Notification (Failure)” and your name was suggested in Chromium Slack as a point of contact.

 

I have a couple of draft CLs that are failing the android-binary-size and Chromium Binary Size bots due to increases in "Normalized APK Size (arm64)," but I'm stumped as to why because the CLs are just moving code from one place to another. I looked at the bot outputs but didn't gain any insights from them. I also tried running `tools/binary_size/diagnose_bloat.py --arm64` as suggested in //docs/speed/binary_size/android_binary_size_trybot.md, but the build for that appears to depend on //chrome/app/theme/google_chrome/BRANDING which I don’t have.

 

Can you help me understand these increases? The CLs are:

https://chromium-review.googlesource.com/c/chromium/src/+/5651218

https://chromium-review.googlesource.com/c/chromium/src/+/5651570

 

Thanks!

Kevin

 

[Andrew Grieve]

Fixed diagnose_bloat.py here.

Also ran it on your CL: results

It shows that blink::RuleFeatureSet::CollectFeaturesFromSelector is +98kb. Maybe some overly aggressive inlining?

This looks similar to this discussion:  where adding a NO_INLINE resolved it: https://chromium-review.googlesource.com/c/chromium/src/+/5534801

On Thu, Jun 27, 2024 at 11:46 AM Kevin Babbitt <kbab...@microsoft.com> wrote:

Sounds good, thank you Andrew!

 

 

From: Andrew Grieve <agr...@chromium.org>
Sent: Thursday, June 27, 2024 8:41 AM
To: Kevin Babbitt <kbab...@microsoft.com>
Cc: agr...@chromium.org; binary-size <binar...@chromium.org>
Subject: Re: Android binary size increase on refactor CLs

 

You don't often get email from agr...@chromium.org. Learn why this is important

[Kevin Babbitt]

Thanks for the quick turnaround on the tooling fix!

 

If blink::RuleFeatureSet::CollectFeaturesFromSelector got bigger due to the compiler changing its inlining decisions, then I’m a little hesitant to second-guess it given that this is a performance sensitive codepath. I admit though I don’t have a good sense for this tradeoff on Android specifically. Adding Rune for his thoughts as well.

 

Another thought: If the bot includes dependent changes in its analysis… the third change in the stack is https://chromium-review.googlesource.com/c/chromium/src/+/5651670 and that CL passed the binary size check with a delta of +3,668 bytes on arm64. Should I interpret that to mean that the compiler changed its inlining decision again and thus the size increase is a temporary effect?

[Daniel Cheng]

Does the binary-size bot build using PGO? I have some vague recollection that it does, and that someone else who encountered similar issues noticed that it went away after the profiles were regenerated.

Daniel

--
You received this message because you are subscribed to the Google Groups "binary-size" group.
To unsubscribe from this group and stop receiving emails from it, send an email to binary-size...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/binary-size/MW4PR00MB1146F44B964A76C49CC70C5CC0D72%40MW4PR00MB1146.namprd00.prod.outlook.com.

[Andrew Grieve]

On Thu, Jun 27, 2024 at 7:52 PM Daniel Cheng <dch...@chromium.org> wrote:

Does the binary-size bot build using PGO? I have some vague recollection that it does, and that someone else who encountered similar issues noticed that it went away after the profiles were regenerated.

It does. We have https://crbug.com/344608183 to track turning it off, although I'm not sure it will actually reduce cases like this (doesn't hurt to try though).

 

Daniel

On Thu, 27 Jun 2024 at 17:42, 'Kevin Babbitt' via binary-size <binar...@chromium.org> wrote:

Thanks for the quick turnaround on the tooling fix!

 

If blink::RuleFeatureSet::CollectFeaturesFromSelector got bigger due to the compiler changing its inlining decisions, then I’m a little hesitant to second-guess it given that this is a performance sensitive codepath. I admit though I don’t have a good sense for this tradeoff on Android specifically. Adding Rune for his thoughts as well.

 

Another thought: If the bot includes dependent changes in its analysis… the third change in the stack is https://chromium-review.googlesource.com/c/chromium/src/+/5651670 and that CL passed the binary size check with a delta of +3,668 bytes on arm64. Should I interpret that to mean that the compiler changed its inlining decision again and thus the size increase is a temporary effect?

I think that is a correct interpretation.