1430534 - Slowly growing 6-7% regression compiler_metrics num_static_constructors

Status: NEW

(Firefox Build System :: General, defect)

Description

[Ionuț Goldan [:igoldan]]

This ~6% regression has gradually increased since November 16th 2017. It affects Linux 32 and 64bit platforms.
Below are the small regressions which got in, linked to their respective offending bug.
I ask each owner to review his bug and consider fixing it, so we can lessen the overall regression.


1% (from 117 to 118) -> Bug 1399413, November 16th, more here https://treeherder.mozilla.org/perf.html#/alerts?id=11178
1% (from 118 to 119) -> Bug 1382574 , November 21st, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11179
1% (from 119 to 120) -> Bug 1290948, November 29th, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11180
1% (from 120 to 121) -> Bug 1423742, December 7th, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11181
1% (from 121 to 122) -> unidentified bug, December 11th, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11183
1% (from 122 to 123) -> Bug 1405877, December 20th, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11175
1% (from 123 to 124) -> Bug 1406231, January 4th 2018, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11182
1% (from 124 to 125) -> Bug 1419771, January 8th 2018, more here
https://treeherder.mozilla.org/perf.html#/alerts?id=11177

Comment 1

[Andrea Marchesini [:baku]]

Bug 1319771 it's making FF faster for the preferences propagation in workers. We have static variables for each important preferences that are needed in workers, but the alternative was to propagate them with runnables to each active worker.

Comment 2

[Masatoshi Kimura [:emk]]

I have no idea why bug 1423742 added an static initializer.

Comment 3

[Byron Campen [:bwc]]

I do not see anything in bug 1290948 that would init something new at static init time (that is what this metric measures, right?), although it did add a new webidl interface, which _might_ end up tripping over this metric? Is there any way to see some output detailing where this test-case is finding problems?

Comment 4

[Ionuț Goldan [:igoldan]]

(In reply to Byron Campen [:bwc] from comment #3)
> (that is what this metric measures, right?)
I believe so.

> Is there any way to see some output detailing where this test-case is finding
> problems?

I'm ni? :froydnj for more details on this, as I am not aware of a way.

Comment 5

[Aria Beingessner [:Gankra]]

Bug 1406231 just deleted dead code. This required some header dependency fixes, could that potentially mess up static initalizers?

Comment 6

[Matthew Gregan [:kinetik]]

I think we're stuck with the static initializer from bug 1405877 for now.  But earlier work for that bug removed a call to CubebUtils::GetCubebContext() very early in child process startup code, so this change is still a net win.

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

In bug 1399413, we use static variables to make it possible to co-use the same hardware module by different content processes. Otherwise, getUserMedia will be broken in multiple e10s modes.

Comment 8

[Alastor Wu [:alwu]]

In bug 1382574, we move all logic about controlling autoplay into a separate class which would tell us whether the media element can start playing when they request the play. The new class has two different static function methods so that we can use them check autoplay without creating a instance.

Comment 9

[Byron Campen [:bwc]]

(In reply to Alexis Beingessner [:Gankro] from comment #5)
> Bug 1406231 just deleted dead code. This required some header dependency
> fixes, could that potentially mess up static initalizers?

If there's a static variable defined in a header file, this could easily happen.

Comment 10

[Ionuț Goldan [:igoldan]]

(In reply to Alexis Beingessner [:Gankro] from comment #5)
> Bug 1406231 just deleted dead code. This required some header dependency
> fixes, could that potentially mess up static initalizers?

I am in no position to explain how that happened. The Perherder test simply confirms there's a regression of this kind.
Please consider to needinfo? other developers familiar with this tech stack, if need be.

Comment 11

[Ionuț Goldan [:igoldan]]

(In reply to Munro Mengjue Chiang [:mchiang] from comment #7)
> In bug 1399413, we use static variables to make it possible to co-use the
> same hardware module by different content processes. Otherwise, getUserMedia
> will be broken in multiple e10s modes.

Sounds like we need to accept the regression from bug 1399413.

Comment 12

[Byron Campen [:bwc]]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #4)
> (In reply to Byron Campen [:bwc] from comment #3)
> > (that is what this metric measures, right?)
> I believe so.
> 
> > Is there any way to see some output detailing where this test-case is finding
> > problems?
> 
> I'm ni? :froydnj for more details on this, as I am not aware of a way.

Until there's a way to see some output that details exactly where these static inits happen, you aren't going to have an easy time fixing the regressions. A number of the comments here (and some code inspection on my part) suggest that there must be header files that do static inits in any translation unit that includes them. You could even see this effect just by adding or removing a cpp file, because it will cause all the cpp files to be re-bucketed in the unified build, which can cause a translation unit to pick up a new header file.

Comment 13

[Nathan Froyd [:froydnj]]

It's not clear to me how bug 1382574 causes static constructors.  But for other bugs (bug 1406231, bug 1290948) that move code around, what likely happened is that the particular files in two unified build files got shifted around a bit, and one of those files that didn't have a static constructor previously now does.  Annoying, but expected.

If we have header files that contain things that cause static initializers, we should fix those, because that's completely unnecessary.

As for other bugs:

(In reply to Matthew Gregan [:kinetik] from comment #6)
> I think we're stuck with the static initializer from bug 1405877 for now. 
> But earlier work for that bug removed a call to
> CubebUtils::GetCubebContext() very early in child process startup code, so
> this change is still a net win.

The comments in the patch say that sIPCConnection is protected by a lock...couldn't we allocate memory at startup, and just have sIPCConnection be a pointer?

(In reply to Munro Mengjue Chiang [:mchiang] from comment #7)
> In bug 1399413, we use static variables to make it possible to co-use the
> same hardware module by different content processes. Otherwise, getUserMedia
> will be broken in multiple e10s modes.

This explanation is unclear to me: sharing data between different content processes would require shared memory, rather than static variables.  What are the static variables buying you in this case?

Looking at the patch, I think the static constructors would come from:

  RefPtr<VideoEngine> CamerasParent::sEngines[CaptureEngine::MaxEngine];

and this could instead be:

  StaticRefPtr<VideoEngine> CamerasParent::sEngines[CaptureEngine::MaxEngine];

to avoid the static constructors.  Could you try making that change?

Comment 14

[Matthew Gregan [:kinetik]]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> The comments in the patch say that sIPCConnection is protected by a
> lock...couldn't we allocate memory at startup, and just have sIPCConnection
> be a pointer?

You're right.  I'll sort this out and post a patch.  Thanks!