1927476 - 0.16% installer size (OSX) regression on Mon October 21 2024

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Treeherder Bug Filer]

Perfherder has detected a build_metrics performance regression from push 45da33804a2d700971eb31c82039a6873de3f9a6. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
0.16%	installer size	osx-cross		99,722,531.92 -> 99,877,292.58

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

You can run all of these tests on try with ./mach try perf --alert 2600

The following documentation link provides more information about this command.

For more information on performance sheriffing please see our FAQ.

If you have any questions, please do not hesitate to reach out to [e-mail].

Comment 1

[Nika Layzell [:nika] (ni? for response)]

It's a bit surprising to me that this had a detectable installer size increase. Perhaps we're missing some code deduplication because of how the callback deserialization is now happening after looking up the callback object. I'll put up a new patch which reduces some of the code duplication and should hopefully reverse this.

Comment 2

[Mayank Bansal]

FWIW (and this may be obvious to you), this also lead to an increase here:

0.4% on XUL section size opt

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

Hey Nika, I am the REO for Fx133 and I am checking in because this bug is marked affecting Fx133. Since you are the assignee are you still planning on fixing this for Fx133? Please note the last day for beta uplifts is Nov 15th.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1927476 - Part 1: Switch ResolveCallback/RejectCallback to be MoveOnlyFunction, r=#ipc-reviewers!

This requires a small change to the filesystem code, as the
FileSystemManager::BeginRequest signature explicitly uses the type
outside of IPC code.

We never need to copy the request object, so using std::function applies
unnecessary restrictions.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1927476 - Part 2: Reduce generated code for IPDL async reply handling, r=#ipc-reviewers!

There was an installer size regression for macOS after the previous
change. I vaguely expect that this was in part caused by moving blocks
of duplicate IPDL serialization/deserialization logic into callback
lambdas, rather than in the larger OnMessageReceived handler function,
potentially leading to less code deduplication.

This patch moves the common parts of the IPDL message deserialization,
for the parts of the message which were serialized by IPDLResolverInner,
into the IPDLAsyncReturnsCallback class.

As a side-effect of this change, things like IPDL message ID validation
and reject case handling has also been moved to generic rather than
generated code.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a9222246491
Part 1: Switch ResolveCallback/RejectCallback to be MoveOnlyFunction, r=ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/484ab36510c0
Part 2: Reduce generated code for IPDL async reply handling, r=ipc-reviewers,mccr8

Comment 7

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/8a9222246491
https://hg.mozilla.org/mozilla-central/rev/484ab36510c0

Comment 8

[Donal Meehan [:dmeehan]]

It looks like this should ride the train with Fx134 and be a wontfix for Fx133.
:nika please let me know if you disagree?

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I agree, I don't think this is a significant enough regression to be uplifted, so it should ride the trains.

(Also I don't believe I've seen an equivalent installer size improvement alert yet (though it may still be too early), so there's always a risk I mis-judged where the regression came from)

Comment 10

[Mayank Bansal]

https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&selected=1457010,1886152136&series=autoland,1457010,1,2&series=autoland,1688129,1,2&series=autoland,1688130,1,2&series=autoland,1688131,1,2&series=autoland,1688132,1,2&series=autoland,1921052,1,2&series=autoland,1921056,1,2&series=autoland,1921061,1,2&series=autoland,1921066,1,2&series=autoland,1921071,1,2&series=autoland,1921076,1,2&series=autoland,2303464,1,2&timerange=1209600&zoom=1730920395143,1730921088247,97923278.63959363,98116015.9888625are better now.