1815177 - 0.61% installer size (OSX) regression on Sat February 4 2023

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Acasandrei Beatrice (needinfo me)]

Perfherder has detected a build_metrics performance regression from push 05923c6cf4552317da8598d3f720a2265898eef7. 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)
1%	installer size	osx-cross		83,188,562.17 -> 83,695,806.42

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 offending patch(es) may be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is probably due to this temporary mix of having old-readparam, then Maybe<> , then old-readparam-like stuff. Nika, I wonder if https://phabricator.services.mozilla.com/D168880 fixes this.

Comment 2

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

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

This is probably due to this temporary mix of having old-readparam, then Maybe<> , then old-readparam-like stuff. Nika, I wonder if https://phabricator.services.mozilla.com/D168880 fixes this.

Hmm, I'm a little bit doubtful, as the new approach does require more code in many cases,but there's a chance it'd improve it slightly? In general this new approach seems like it might require more copies when deserializing data structures due to the need to use a Maybe<T> temporary object, as well as that it will need to handle destructors in many more places, rather than only in one place, with individual deserializers rarely needing to manually invoke destructors.

I made a mocked out version of the change in godbolt, assuming everything switches over to use std::optional<T> temporaries, and you can see just how much more code is generated in that case (https://godbolt.org/z/33Knr9je5). The old approach (on x86-64) only generates ~37 lines of assembly, compared to the new approach's ~100 lines.

I'm a bit worried that this might be a structural issue with the new approach, as I'm not sure how we'd get around it, and I worry a bit that it could hurt deserialization performance.

Comment 3

[Ray Kraesig [:rkraesig]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

I made a mocked out version of the change in godbolt, assuming everything switches over to use std::optional<T> temporaries, and you can see just how much more code is generated in that case (https://godbolt.org/z/33Knr9je5). The old approach (on x86-64) only generates ~37 lines of assembly, compared to the new approach's ~100 lines.

I got nerd-sniped, so: have you considered performing subobject destruction explicitly (https://godbolt.org/z/rs76qE16G), rather than relegating it to helper objects?

Comment 4

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

Looking at a try push for a (busted, but hopefully representative code-size wise) version of my patch stack, it looks like after my changes in bug 1814683, bug 1607634 and bug 1814686 the OS X installer size opt from my one push is on the lower end of the new range, but clearly isn't back to where the installer size used to be (https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&highlightedRevisions=178eedfaf19dfaadabaacc9dcc8c051df9fc19a4&series=try,1513970,1,2&timerange=1209600&zoom=1675383040972,1675983323997,82108200.13702366,84667386.46801017).

Comment 5

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

(In reply to Ray Kraesig [:rkraesig] from comment #3)

I got nerd-sniped, so: have you considered performing subobject destruction explicitly (https://godbolt.org/z/rs76qE16G), rather than relegating it to helper objects?

Doing something like that would technically be an option, but at the end of the day we have a lot of manually implemented ParamTraits methods, and we need to support manually implemented ones which return types without default constructors in order to implement these things. Requiring folks to work with uninitialized outparameters for a type like that feels a bit unergonomic, and we'd still have issues with needing to invoke destructors in failure cases for members which require the outparameter to be initialized.

There might be something along this line which we could do, but figuring out how to do it in an ergonomic way which also integrates with existing ParamTraits impls (because rewriting all of them is unfortunately a bit impractical) might be tricky.

Comment 6

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

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

Looking at a try push for a (busted, but hopefully representative code-size wise) version of my patch stack, it looks like after my changes in bug 1814683, bug 1607634 and bug 1814686 the OS X installer size opt from my one push is on the lower end of the new range, but clearly isn't back to where the installer size used to be (https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&highlightedRevisions=178eedfaf19dfaadabaacc9dcc8c051df9fc19a4&series=try,1513970,1,2&timerange=1209600&zoom=1675383040972,1675983323997,82108200.13702366,84667386.46801017).

I tried out another approach where I use a fake Maybe-like type in situations where we're using the outparameter which never uninitializes its storage, and it seems like that has largely reduced the installer size back to a more normal level (https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&highlightedRevisions=b71f9aa2141661d3a8edbe052cf1c83d27f0aed1&series=try,1513970,1,2&timerange=1209600&zoom=1675477342532,1675995241923,81742602.08973986,84667386.46801017) - this doesn't do any of the fancy things with struct partial initialization or anything like that, it still invokes move constructors for those cases. I think this might be a somewhat reasonable approach to take, perhaps with a more consistent type between the two cases rather than faking out the Maybe interface just enough for code to compile.

Comment 7

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

Attachment: Bug 1815177 - Use a custom ReadResult to reduce branching during IPDL deserialization, r=#ipc-reviewers!

Depends on D168890

Comment 8

[Donal Meehan [:dmeehan]]

:nika I see you set the severity as S2, does that indicate there's a bigger problem than the installer size increase?
Would like to know if this should be tracked for 111 and potential uplifts OR if it can ride the train with 112 whenever that patch lands

Comment 9

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

(In reply to Donal Meehan [:dmeehan] from comment #8)

:nika I see you set the severity as S2, does that indicate there's a bigger problem than the installer size increase?
Would like to know if this should be tracked for 111 and potential uplifts OR if it can ride the train with 112 whenever that patch lands

Nope, it was just me autopiloting because I had assigned it to myself. I think the only real problem is the installer size increase. I think it's probably fine to ride the trains.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 11

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

There are dependencies which haven't fully been reviewed yet. Will land when that's ready.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae1c0551cea5
Use a custom ReadResult to reduce branching during IPDL deserialization, r=ipc-reviewers,mccr8

Comment 13

[Iulian Moraru]

Backed out (bug 1607634, bug 1814683, bug 1815177, bug 1814686) for causing build bustages on MaybeStorageBase.

• Push with failures

• Failure log

• Backout link

[task 2023-03-14T22:45:19.685Z] 22:45:19     INFO -                   from Unified_cpp_dom_ipc5.cpp:2:
[task 2023-03-14T22:45:19.686Z] 22:45:19     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/MaybeStorageBase.h: In instantiation of 'constexpr mozilla::detail::MaybeStorageBase<T, true>::Union::Union(std::in_place_t, Args&& ...) [with Args = {mozilla::NotNull<mozilla::ipc::SideVariant<mozilla::dom::PBrowserParent*, mozilla::dom::PBrowserChild*> >, int}; T = mozilla::dom::PopupIPCTabContext]':
[task 2023-03-14T22:45:19.687Z] 22:45:19     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/MaybeStorageBase.h:84:61:   required from 'constexpr mozilla::detail::MaybeStorageBase<T, true>::MaybeStorageBase(std::in_place_t, Args&& ...) [with Args = {mozilla::NotNull<mozilla::ipc::SideVariant<mozilla::dom::PBrowserParent*, mozilla::dom::PBrowserChild*> >, int}; T = mozilla::dom::PopupIPCTabContext]'
[task 2023-03-14T22:45:19.688Z] 22:45:19     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:288:21:   required from 'constexpr mozilla::detail::MaybeStorage<T, true>::MaybeStorage(std::in_place_t, Args&& ...) [with Args = {mozilla::NotNull<mozilla::ipc::SideVariant<mozilla::dom::PBrowserParent*, mozilla::dom::PBrowserChild*> >, int}; T = mozilla::dom::PopupIPCTabContext]'
[task 2023-03-14T22:45:19.689Z] 22:45:19     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Maybe.h:378:76:   required from 'constexpr mozilla::Maybe<T>::Maybe(std::in_place_t, Args&& ...) [with Args = {mozilla::NotNull<mozilla::ipc::SideVariant<mozilla::dom::PBrowserParent*, mozilla::dom::PBrowserChild*> >, int}; T = mozilla::dom::PopupIPCTabContext]'
[task 2023-03-14T22:45:19.690Z] 22:45:19     INFO -  /builds/worker/checkouts/gecko/ipc/chromium/src/chrome/common/ipc_message_utils.h:321:58:   required from 'IPC::ReadResult<T, false>::ReadResult(std::in_place_t, Args&& ...) [with Args = {mozilla::NotNull<mozilla::ipc::SideVariant<mozilla::dom::PBrowserParent*, mozilla::dom::PBrowserChild*> >, int}; T = mozilla::dom::PopupIPCTabContext]'
[task 2023-03-14T22:45:19.690Z] 22:45:19     INFO -  /builds/worker/workspace/obj-build/ipc/ipdl/PTabContext.cpp:60:14:   required from here
[task 2023-03-14T22:45:19.691Z] 22:45:19    ERROR -  /builds/worker/workspace/obj-build/dist/include/mozilla/MaybeStorageBase.h:71:43: error: narrowing conversion of 'std::forward<int>((* & aArgs#1))' from 'int' to 'mozilla::dom::PopupIPCTabContext::uint64_t {aka long unsigned int}' inside { } [-Werror=narrowing]
[task 2023-03-14T22:45:19.691Z] 22:45:19     INFO -           : val{std::forward<Args>(aArgs)...} {}
[task 2023-03-14T22:45:19.692Z] 22:45:19     INFO -                                             ^
[task 2023-03-14T22:45:19.692Z] 22:45:19     INFO -  cc1plus: all warnings being treated as errors
[task 2023-03-14T22:45:19.693Z] 22:45:19    ERROR -  gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:670: Unified_cpp_dom_ipc5.o] Error 1
[task 2023-03-14T22:45:19.693Z] 22:45:19     INFO -  gmake[4]: Leaving directory '/builds/worker/workspace/obj-build/dom/ipc'

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40837f556c95
Use a custom ReadResult to reduce branching during IPDL deserialization, r=ipc-reviewers,mccr8

Comment 15

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/40837f556c95

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

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

I don't think that uplifting this change is a good idea. Due to the dependencies, it is quite a substantial change.