1513179 - 0.22 - 0.24% installer size (linux64, windows2012-32) regression on push d321a6c5e0718dc0f8e26c567555ed296c751d9e (Thu Nov 29 2018)

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect)

Description

[Florin Strugariu [:Bebe]]

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=d321a6c5e0718dc0f8e26c567555ed296c751d9e

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  0%  installer size windows2012-32 pgo      65,388,869.50 -> 65,548,996.25
  0%  installer size linux64 pgo             68,226,412.21 -> 68,374,472.25


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=17903

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Automated_Performance_Testing_and_Sheriffing/Build_Metrics

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Comment 1

[Boris Zbarsky [:bzbarsky]]

That patch just removes a bunch of code.

It's possible that with the code removed compilers make different inlining decisions about the function the code was in, leading to a larger binary, of course...  The fact that it's only PGO builds that are affected certainly points in that direction.

Poking around with bloaty, it says that .text got about 500KB bigger, which is consistent with larger codesize.

Looking at the output of "readelf -sW libxul.so" on Linux, I see differences like this:

old: 

  241871: 0000000004457b70  1350 FUNC    LOCAL  DEFAULT   16
  mozilla::dom::Document_Binding::CreateInterfaceObjects(JSContext*,
    JS::Handle<JSObject*>, mozilla::dom::ProtoAndIfaceCache&, bool)

new:

  242198: 0000000004501bd0  2151 FUNC    LOCAL  DEFAULT   16
  mozilla::dom::Document_Binding::CreateInterfaceObjects(JSContext*,
    JS::Handle<JSObject*>, mozilla::dom::ProtoAndIfaceCache&, bool)

So that function went from 1350 bytes to 2151 bytes.  We have lots of these CreateInterfaceObjects functions.

CreateInterfaceObjects calls dom::CreateInterfaceObjects (which is large, presumably not inlined, and got smaller with the changes) and GetProtoObjectHandle/GetConstructorObjectHandle, which _are_ marked inline.  Looking at just those, old:

  60986: 0000000004457b50    22 FUNC    LOCAL  DEFAULT   16
  mozilla::dom::Document_Binding::GetProtoObjectHandle(JSContext*)

new:

  61108: 0000000004501ac0   262 FUNC    LOCAL  DEFAULT   16
  mozilla::dom::Document_Binding::GetProtoObjectHandle(JSContext*)

So it went from 22 to 262 bytes, and is presumably still getting inlined.  Those functions call GetPerInterfaceObjectHandle which looks like this, old:

  225844: 0000000003b5d280   565 FUNC    LOCAL  DEFAULT   16
  mozilla::dom::GetPerInterfaceObjectHandle(JSContext*, unsigned long,
    void (*)(JSContext*, JS::Handle<JSObject*>,
    mozilla::dom::ProtoAndIfaceCache&, bool), bool)

new:

  226182: 0000000003c1f3d0   312 FUNC    LOCAL  DEFAULT   16
  mozilla::dom::GetPerInterfaceObjectHandle(JSContext*, unsigned long,
    void (*)(JSContext*, JS::Handle<JSObject*>,
    mozilla::dom::ProtoAndIfaceCache&, bool), bool)

which is _smaller_ (as expected), but hence more likely to get inlined.

So my guess is that mozilla::dom::GetPerInterfaceObjectHandle used to not get inliend into GetProtoObjectHandle/GetConstructorObjectHandle because it was too big.  Now it's smaller, getting inlined, and leading to larger codesize as a result.  But only in PGO builds, which suggests the profiler decided it was worth inlining.