Closed Bug 1454592 Opened 6 years ago Closed 6 years ago

1.59 - 1.59% compiler_metrics num_static_constructors (linux32, linux64) regression on push a434fac58370fc14bceb525e1ca8b7125638b6cd (Sat Apr 14 2018)

Categories

(Core :: JavaScript Engine, defect)

Unspecified
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox61 - fixed

People

(Reporter: igoldan, Assigned: jandem)

References

Details

(Keywords: regression)

Attachments

(2 files)

We have detected a build metrics regression from push:

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

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

Regressions:

  2%  compiler_metrics num_static_constructors linux32 pgo      126.00 -> 128.00
  2%  compiler_metrics num_static_constructors linux64 pgo      126.00 -> 128.00
  2%  compiler_metrics num_static_constructors linux32 opt      126.00 -> 128.00


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

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
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
:jandem Please look over bug 1452982 and address the compiler_metrics regressions.
Flags: needinfo?(jdemooij)
This is likely because of the gc/ZoneGroup.cpp removal affecting unified builds :/ The other patches in that push are mostly just renaming stuff.

I can do some Try pushes to confirm this, but it doesn't look like there's an easy fix.
This looks like a unified build issue.

Here's some cleanup/constexpr-ifying of various (static) constructors - it seems to get us from 128 to 127 static constructors.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8969587 - Flags: review?(jcoppeard)
I wanted to mark the protoTable (a static array) in GlobalObject.cpp as constexpr, but for that to work I had to remove ProxyClassPtr and instead expose ProxyClass directly. That's actually a bit simpler too.
Attachment #8969588 - Flags: review?(jcoppeard)
Comment on attachment 8969587 [details] [diff] [review]
Part 1 - Make some constructors constexpr

Review of attachment 8969587 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/StoreBuffer.h
@@ +530,5 @@
> +      , next(nullptr)
> +#ifdef DEBUG
> +      , minorGCNumberAtCreation(0)
> +#endif
> +    { }

What's the style for empty blocks?  We use {} in most places I think.

Oh hmmm, it seems we use both.
Attachment #8969587 - Flags: review?(jcoppeard) → review+
Comment on attachment 8969588 [details] [diff] [review]
Part 2 - More constexpr

Review of attachment 8969588 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/ProxyObject.h
@@ -129,5 @@
>      void nuke();
> -
> -    // There is no class_ member to force specialization of JSObject::is<T>().
> -    // The implementation in JSObject is incorrect for proxies since it doesn't
> -    // take account of the handler type.

Should we keep this comment?
Attachment #8969588 - Flags: review?(jcoppeard) → review+
(In reply to Jon Coppeard (:jonco) from comment #5)
> What's the style for empty blocks?  We use {} in most places I think.
> 
> Oh hmmm, it seems we use both.

Yeah, we use both... I usually use {} though so I'll just change it.

(In reply to Jon Coppeard (:jonco) from comment #6)
> Should we keep this comment?

Hm maybe we should - I can move it to the top of the ProxyObject class so people are more likely to see it.
According to Try, these patches should get us back to 126 static constructors. Again, note that I'm just fixing some pre-existing issues; unified build changes exposed this somehow...
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89504aa6f1b3
part 1 - Make some constructors constexpr. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/82821ff143e3
part 2 - Use constexpr for protoTable static array. r=jonco
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0c62242c48
Backed out 2 changesets for Windows build bustage on a CLOSED TREE
This looks like an MSVC constexpr bug :( The protoTable case is very similar to the bug described in bug 1445089 comment 14.

Maybe we can just keep the table as const instead of constexpr and hope compilers are smart enough...
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15ad0dfe3a38
part 1 - Make some constructors constexpr. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/fde4d35d666a
part 2 - Get rid of a static constructor for protoTable array. r=jonco
https://hg.mozilla.org/mozilla-central/rev/15ad0dfe3a38
https://hg.mozilla.org/mozilla-central/rev/fde4d35d666a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I confirm the fix:

== Change summary for alert #12906 (as of Wed, 25 Apr 2018 05:07:24 GMT) ==

Improvements:

  2%  compiler_metrics num_static_constructors linux32 pgo      127.50 -> 125.00
  2%  compiler_metrics num_static_constructors linux64 pgo      127.50 -> 125.00
  2%  compiler_metrics num_static_constructors linux32 opt      127.50 -> 125.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12906
You need to log in before you can comment on or make changes to this bug.