1692403 - 0.09 - 0.1% installer size (osx-shippable) regression on push d092f5abddc8330069df373d4610bcc891d4f4dc (Tue February 9 2021)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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

Regressions:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
0.10%	installer size		osx-shippable	instrumented	114,050,084.08 -> 114,161,399.17
0.09%	installer size		osx-shippable	instrumented	114,052,634.58 -> 114,154,319.08

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) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Comment 1

[Simon Giesecke [:sg] [he/him]]

While I am surprised that the changes come with such an effect, Bug 1691894 is going to change a lot of the sites again. This might or might not reverse the installer size increase.

If that's caused by the changes, this must correlate with a relatively even larger increase of libxul text section size. Is no alert filed for that? I also wonder if only OS X is affected.

I could look at the generated code in detail, but I can only easily do so on Linux.

Comment 2

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

If that's caused by the changes, this must correlate with a relatively even larger increase of libxul text section size. Is no alert filed for that? I also wonder if only OS X is affected.

Haven't seen any libxul alerts. Should this be under build metrics as well?

The other OS are slightly affected, but not over the alerting threshold.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Yes, also under build_metrics: XUL section sizes opt on the *-shippable builds.

Comment 4

[Simon Giesecke [:sg] [he/him]]

So, some of the patches of Bug 1691894 are have landed resp. are landing now. Can you check if this reverts the regression?

I am inclined to say that the changes should not be backed out in any case.

But we might investigate further if we can improve the implementation of the new API to reduce the effects on the code/installer size.

Comment 5

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

I am inclined to say that the changes should not be backed out in any case.

Got it

Can you check if this reverts the regression?

Nothing that reverts the regression I can see yet.

Comment 6

[Simon Giesecke [:sg] [he/him]]

I looked at the generated binaries (on Linux) now. Unfortunately it's somewhat hard to tell what's actually different since there are many changes in symbol names (clone ids). One thing that seems to have consistently grown are the Wrap and _finalize functions in the generated WebIDL bindings, such as:

  +1.5%     +22  +1.7%     +22    mozilla::dom::NotificationEvent_Binding::Wrap()
  +6.6%     +22  +9.4%     +22    mozilla::dom::NotificationEvent_Binding::_finalize()

It's not much but there are a lot of instances of those. I'll have a closer look what code is added there.

Comment 7

[Simon Giesecke [:sg] [he/him]]

Something changed related to inlining of DeferredFinalize:

< mov    %r15,%rsi
< callq  0x3da8ac0 <mozilla::CycleCollectedJSRuntime::DeferredFinalize(nsISupports*)>
---
> lea    -0x12dc221(%rip),%rsi        # 0x3da8a60 <mozilla::dom::DeferredFinalizerImpl<nsISupports>::AppendDeferredFinalizePointer(void*, void*)>
> lea    -0x12dc188(%rip),%rdx        # 0x3da8b00 <mozilla::dom::DeferredFinalizerImpl<nsISupports>::DeferredFinalize(unsigned int, void*)>
> mov    %r15,%rcx
> callq  0x3da8960 <mozilla::CycleCollectedJSRuntime::DeferredFinalize(void* (*)(void*, void*), bool (*)(unsigned int, void*), void*)>

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

:sg anything else we can do here or should we accept this regression?

Comment 9

[Simon Giesecke [:sg] [he/him]]

It's possible that something could be done to reduce the effect of using *Hashtable::WithEntryHandle, but I am not sure if it's worth the effort. Nika, what do you think?

Comment 10

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

(In reply to Simon Giesecke [:sg] [he/him] from comment #7)

Something changed related to inlining of DeferredFinalize:

Hm, so in DeferredFinalize we definitely use WithEntryHandle to insert the entry to be created (https://searchfox.org/mozilla-central/rev/87a8afd9f57ee4bc542ba0ec3f96a891042b6db7/xpcom/base/CycleCollectedJSRuntime.cpp#1544-1550). That particular change though doesn't seem very substantial, as it looks like some slightly different calls were made due to inlining there (probably thanks to PGO noise?) but the actual use of the WithEntryHandle API doesn't appear to have been inlined, so I'm a bit surprised the change is so significant.

Re: the change to mozilla::dom::NotificationEvent_Binding::Wrap(), the _finalize() function is only called in an error case, so it might be worth making those methods MOZ_NEVER_INLINE so that it isn't inlined into Wrap(), inflating the size of that method. That probably requires threading a neverInline flag from CGAbstractClassHook (which should probably never be inlined?) down to CGAbstractMethod, like we do for MOZ_ALWAYS_INLINE.

Other than that, I suppose I can defer to :mccr8 who might have more insight into the cycle-collector and other stuff here?

(In reply to Simon Giesecke [:sg] [he/him] from comment #9)

It's possible that something could be done to reduce the effect of using *Hashtable::WithEntryHandle, but I am not sure if it's worth the effort. Nika, what do you think?

The main thing I can think of which is more code after WithEntryHandle which wasn't around before is probably the change to ~EntryHandle() to call ShrinkIfAppropriate() if the entry was removed or not inserted (https://searchfox.org/mozilla-central/rev/f6ffb71dca9eb491e85aa95042380b2602008b00/xpcom/ds/PLDHashTable.cpp#708). I'm guessing that code very rarely actually shrinks the size of the table, and I could see many more calls to that function being inserted in various places where they wouldn't have been otherwise, basically everywhere where we use the method.

The previous LookupForAdd interface wouldn't shrink the table if the entry was removed after being found, which is also more similar to the behaviour of e.g. Iterator::Remove, so perhaps we can get away with removing it if folks generally aren't using WithEntryHandle as the primary way to remove entries from the hashtable, which might in turn reduce code size in places which call it.

Comment 11

[Andrew McCreight (out of office until 8/21) [:mccr8]]

I think Peter wrote this finalization stuff, but it seems okay to me to put a never inline on DeferredFinalize. It isn't going to be so performance sensitive that it matters, I'm guessing.

Comment 12

[Simon Giesecke [:sg] [he/him]]

Thanks for the suggestions, Nika and Andrew!

I checked both things.on a Linux opt LTO, non-PGO build.

Removing the ShrinkIfAppropriate call from the WithEntryHandle dtor leads to a slight text segment reduction of 4.92K

Marking void DeferredFinalize(nsISupports* aSupports) as MOZ_NEVER_INLINE leads to a more significant text segment reduction of 48.5K

I attached patches for both changes, they are independent from each other.

Comment 13

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1692403 - Don't shrink hashtable in EntryHandle destructor. r=#xpcom-reviewers

Comment 14

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1692403 - Never inline DeferredFinalize(nsISupports*). r=#xpcom-reviewers

Comment 15

[Simon Giesecke [:sg] [he/him]]

Perfherder results of try push with shippable builds: https://treeherder.mozilla.org/perfherder/compare?originalProject=mozilla-central&originalRevision=d0238d81621ef6c0377b7328e71bf7c7ce8a610f&newProject=try&newRevision=3066b287b062deb538eea63e64c149d5ef36eae6&framework=2

This doesn't load for me for some reason though.

I tried to manually inspect the OS X installer size and if I am not mistaken, both patches combined reduce the size by ca. 86k

Comment 16

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Seems like the results of the try push bring the desired improvements. I used the graph view because for some reason the comparison view doesn't load for me either.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0e74a095d9f
Never inline DeferredFinalize(nsISupports*). r=xpcom-reviewers,mccr8

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b0e74a095d9f

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a7c8d87d7ce
Don't shrink hashtable in EntryHandle destructor. r=xpcom-reviewers,nika

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8a7c8d87d7ce