Closed Bug 1515002 Opened Last year Closed Last year

0.5 - 0.93% compiler_metrics num_static_constructors (windows2012-32, windows2012-64, windows2012-aarch64) regression on push d09d3161572aecefd2e47b574e3a14ea8c6ee1d6 (Mon Dec 17 2018)

Categories

(Core :: WebVR, defect)

All
Windows
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed

People

(Reporter: igoldan, Assigned: daoshengmu)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

We have detected a build metrics regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=331a2a22924c30743e356526e3a8c67bc866fca4&tochange=d09d3161572aecefd2e47b574e3a14ea8c6ee1d6

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

Regressions:

  1%  compiler_metrics num_static_constructors windows2012-32 opt msvc     535.00 -> 540.00
  1%  compiler_metrics num_static_constructors windows2012-aarch64 opt msvc-aarch64     608.00 -> 613.00
  1%  compiler_metrics num_static_constructors windows2012-64 opt msvc             626.00 -> 631.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=18272

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! ***
Component: General → WebVR
Product: Testing → Core
Version: 61 Branch → unspecified
Flags: needinfo?(dmu)
(In reply to Daosheng Mu[:daoshengmu] from comment #1)
> Created attachment 9032250 [details]
> Bug 1515002 - Fixing compiler treats OpenVR controller bindings as static
> constructors.

Hi Ionuț,
Could you try my patch from attachment 9032250 [details] to see if it gets improvement.
Thanks,
Flags: needinfo?(dmu) → needinfo?(igoldan)
I didn't see any static constructor from my previous patches, I doubt it is because the compiler treats the constant variable in my OpenVR controller binding header files as static variable and be initialized at the construction time. I have changed it to the singleton approach, hope it would get better.
These variables sure look like they would cause static constructors:

https://hg.mozilla.org/integration/autoland/rev/d09d3161572a#l2.142
Attachment #9032250 - Attachment is obsolete: true
(In reply to Daosheng Mu[:daoshengmu] from comment #2)
> (In reply to Daosheng Mu[:daoshengmu] from comment #1)
> > Created attachment 9032250 [details]
> > Bug 1515002 - Fixing compiler treats OpenVR controller bindings as static
> > constructors.
> 
> Hi Ionuț,
> Could you try my patch from attachment 9032250 [details] to see if it gets
> improvement.
> Thanks,

I think attachment 9032297 [details] would be a right fix. Ionuț, please try this one.

And big thanks to :froydnj for helping figure it out.
No longer depends on: 1515261
(In reply to Daosheng Mu[:daoshengmu] from comment #6)
> (In reply to Daosheng Mu[:daoshengmu] from comment #2)
> > (In reply to Daosheng Mu[:daoshengmu] from comment #1)
> > > Created attachment 9032250 [details]
> > > Bug 1515002 - Fixing compiler treats OpenVR controller bindings as static
> > > constructors.
> > 
> > Hi Ionuț,
> > Could you try my patch from attachment 9032250 [details] to see if it gets
> > improvement.
> > Thanks,
> 
> I think attachment 9032297 [details] would be a right fix. Ionuț, please try
> this one.

I'm doing the checks right now.
Attachment #9032297 - Attachment is obsolete: true
(In reply to Daosheng Mu[:daoshengmu] from comment #9)
> Created attachment 9032499 [details]
> Bug 1515002 - Avoid OpenVR controllerInfo and manifest files be instanced in
> global to make a lot of static constructors.

Hi Ionuț,
Do you mind to help try it again? I believe this is a right fix.

Thanks :)
Flags: needinfo?(igoldan)
(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> (In reply to Daosheng Mu[:daoshengmu] from comment #9)
> > Created attachment 9032499 [details]
> > Bug 1515002 - Avoid OpenVR controllerInfo and manifest files be instanced in
> > global to make a lot of static constructors.
> 
> Hi Ionuț,
> Do you mind to help try it again? I believe this is a right fix.
> 
> Thanks :)

Indeed, this completely fixed the regressions on 32 and 64 bit architectures. But on AARCH64 we're still left with 4 extra static constructors.
Flags: needinfo?(igoldan)
Ignore the yellow text wall.
(In reply to Ionuț Goldan [:igoldan][PTO Dec 21-Jan 2], Performance Sheriffing from comment #11)
> (In reply to Daosheng Mu[:daoshengmu] from comment #10)
> > (In reply to Daosheng Mu[:daoshengmu] from comment #9)
> > > Created attachment 9032499 [details]
> > > Bug 1515002 - Avoid OpenVR controllerInfo and manifest files be instanced in
> > > global to make a lot of static constructors.
> > 
> > Hi Ionuț,
> > Do you mind to help try it again? I believe this is a right fix.
> > 
> > Thanks :)
> 
> Indeed, this completely fixed the regressions on 32 and 64 bit
> architectures. But on AARCH64 we're still left with 4 extra static
> constructors.

Not quite understand why this change doesn't affect AARCH64. I think the StaticRefPtr<ControllerManifestFile> xxx should be the correct way to fix.

:froydnj, do you have idea about AARCH64?  Thanks.
Flags: needinfo?(nfroyd)
(In reply to Ionuț Goldan [:igoldan][PTO Dec 21-Jan 2], Performance Sheriffing from comment #11)
> (In reply to Daosheng Mu[:daoshengmu] from comment #10)
> > (In reply to Daosheng Mu[:daoshengmu] from comment #9)
> > > Created attachment 9032499 [details]
> > > Bug 1515002 - Avoid OpenVR controllerInfo and manifest files be instanced in
> > > global to make a lot of static constructors.
> > 
> > Hi Ionuț,
> > Do you mind to help try it again? I believe this is a right fix.
> > 
> > Thanks :)
> 
> Indeed, this completely fixed the regressions on 32 and 64 bit
> architectures. But on AARCH64 we're still left with 4 extra static
> constructors.

Well, for opt msvc-aarch64, the static constructors are decreased from 613.00 > 608.00, only "debug" msvc-aarch64 still has four static constructors. It seems like the compiler non-opt version doesn't do well for StaticRefPtr<ControllerManifestFile> in msvc-aarch64. I am curious how is the result in 32-bit debug msvc and 64-bit debug msvc ? I guess both of them will have the similar result as "debug" msvc-aarch64.  

I think we are good to land this patch :)
:kip,
I would like to suggest you just review the version of https://phabricator.services.mozilla.com/D15004?vs=on&id=46041&whitespace=ignore-most#toc because the clang format warning makes me do the following commit to correct the coding style by "./mach clang-format -p PATH"
(In reply to Daosheng Mu[:daoshengmu] from comment #15)
> (In reply to Ionuț Goldan [:igoldan][PTO Dec 21-Jan 2], Performance
> Sheriffing from comment #11)
> > (In reply to Daosheng Mu[:daoshengmu] from comment #10)
> > > (In reply to Daosheng Mu[:daoshengmu] from comment #9)
> > > > Created attachment 9032499 [details]
> > > > Bug 1515002 - Avoid OpenVR controllerInfo and manifest files be instanced in
> > > > global to make a lot of static constructors.
> > > 
> > > Hi Ionuț,
> > > Do you mind to help try it again? I believe this is a right fix.
> > > 
> > > Thanks :)
> > 
> > Indeed, this completely fixed the regressions on 32 and 64 bit
> > architectures. But on AARCH64 we're still left with 4 extra static
> > constructors.
> 
> Well, for opt msvc-aarch64, the static constructors are decreased from
> 613.00 > 608.00, only "debug" msvc-aarch64 still has four static
> constructors. It seems like the compiler non-opt version doesn't do well for
> StaticRefPtr<ControllerManifestFile> in msvc-aarch64. I am curious how is
> the result in 32-bit debug msvc and 64-bit debug msvc ? I guess both of them
> will have the similar result as "debug" msvc-aarch64.  
> 
> I think we are good to land this patch :)

For the debug version of StaticRefPtr<> at https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/xpcom/base/StaticPtr.h#120, we have a non-trival constructor. So, that is the reason why we still have these four static constructors. I think it is intentional.
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9561841251e7
Avoid OpenVR controllerInfo and manifest files be instanced in global to make a lot of static constructors. r=kip
https://hg.mozilla.org/mozilla-central/rev/9561841251e7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
I don't know why it would be so different on aarch64, but if the numbers only change for debug, I'm not too worried about it.
Flags: needinfo?(nfroyd)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.