Closed Bug 579131 Opened 12 years ago Closed 11 years ago

Keep track of .ctors to fight global initializers

Categories

(Release Engineering :: General, defect, P4)

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: taras.mozilla, Assigned: catlee)

References

Details

(Whiteboard: [ts])

Attachments

(6 files, 2 obsolete files)

I posted about this
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/abf60ad5fd03a708?pli=1

Basic idea is that we need to keep track of .ctors size for libxul.so.

then number is in 
readelf -S libxul.so
in the size column. We need to have a regression/improvement email every time that number changes. This would be done on linux and maybe possibly OSX. I'm not yet sure what the equivalent is on Windows. 

I think this is high-ish priority as this should be pretty easy and it would start catching regressions immediately.
Whiteboard: [ts]
As alternative implementation I'd suggest that this be TinderboxPrint'ed and sent to the graph server, like we do with codesighs/leak tests numbers. It can then be monitored the same way those tests are.
Dumpbin may be able to do the same on Windows.
Depends on: 551531
Assignee: nobody → catlee
Priority: -- → P3
Blocks: 569629
Attachment #460588 - Flags: review?(bhearsum)
Attachment #460588 - Flags: feedback?(tglek)
Attachment #460589 - Flags: feedback?(bhearsum)
Comment on attachment 460589 [details] [diff] [review]
Count global constructors on linux

This looks like it'll count the ctors, but I don't see them being TinderboxPrint'ed or sent to the graph server. What's the plan for monitoring these?
Attachment #460589 - Flags: feedback?(bhearsum) → feedback-
(In reply to comment #5)
> Comment on attachment 460589 [details] [diff] [review]
> Count global constructors on linux
> 
> This looks like it'll count the ctors, but I don't see them being
> TinderboxPrint'ed or sent to the graph server. What's the plan for monitoring
> these?

I can TinderboxPrint it pretty easily.  Graph server posting will have to wait until Rail's work on doing that on the slaves is finished, which will make posting this data super easy.
Comment on attachment 460588 [details] [diff] [review]
Script to return the global constructor count on linux

Looks fine to me
Attachment #460588 - Flags: review?(bhearsum) → review+
(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 460589 [details] [diff] [review] [details]
> > Count global constructors on linux
> > 
> > This looks like it'll count the ctors, but I don't see them being
> > TinderboxPrint'ed or sent to the graph server. What's the plan for monitoring
> > these?
> 
> I can TinderboxPrint it pretty easily.  Graph server posting will have to wait
> until Rail's work on doing that on the slaves is finished, which will make
> posting this data super easy.

WFM
> I can TinderboxPrint it pretty easily.  Graph server posting will have to wait
> until Rail's work on doing that on the slaves is finished, which will make
> posting this data super easy.

I'm mostly interested in regression/improvement emails, will those be available before graph server?
(In reply to comment #9)
> > I can TinderboxPrint it pretty easily.  Graph server posting will have to wait
> > until Rail's work on doing that on the slaves is finished, which will make
> > posting this data super easy.
> 
> I'm mostly interested in regression/improvement emails, will those be available
> before graph server?

No
Attachment #460589 - Attachment is obsolete: true
Attachment #460660 - Flags: review?(bhearsum)
Comment on attachment 460660 [details] [diff] [review]
Count global constructors on linux

Cool!
Attachment #460660 - Flags: review?(bhearsum) → review+
Comment on attachment 460588 [details] [diff] [review]
Script to return the global constructor count on linux

changeset:   709:6c1a3110a600
Attachment #460588 - Flags: checked-in+
Comment on attachment 460660 [details] [diff] [review]
Count global constructors on linux

changeset:   846:8372cb6ea0d9
Attachment #460660 - Flags: checked-in+
Depends on: 555205
Priority: P3 → P4
Attachment #460588 - Flags: feedback?(tglek)
is there an eta for getting warning emails on this?
(In reply to comment #15)
> is there an eta for getting warning emails on this?

nope; everybody is tied up with q3 goals, and the dependent bug for this needs to be fixed first.
Here is a modification that will return the exact number of ctors (not a number of bytes of the section, but an actual count). It should also work on arm-based ports (maemo, android), where the ctors are in a .init_array section.
Attachment #489476 - Flags: review?(bhearsum)
Comment on attachment 489476 [details] [diff] [review]
Enhance the script counting the number of ctors on linux

Please leave it us to land this. It affects infra, so we need to do it in a scheduled reconfig window.
Attachment #489476 - Flags: review?(bhearsum) → review+
Flags: needs-reconfig?
Comment on attachment 489476 [details] [diff] [review]
Enhance the script counting the number of ctors on linux

changeset:   1818:0ee69e37e575
Attachment #489476 - Flags: checked-in+
Nothing to check in here for now. Removing needs-reconfig flag.
Flags: needs-reconfig?
Depends on: 619006
Depends on: 619975
No longer depends on: 619006
Attachment #508914 - Flags: review?(bhearsum)
We need a new name in the tests table so graph server knows where to put stuff.
Attachment #508916 - Flags: review?(bear)
Attachment #508914 - Flags: review?(bhearsum) → review+
Attachment #508916 - Flags: review?(bear) → review+
Flags: needs-reconfig?
Flags: needs-reconfig?
Comment on attachment 508914 [details] [diff] [review]
Send constructors count to graph server

http://hg.mozilla.org/build/buildbotcustom/rev/70277cb3fb30
Attachment #508914 - Flags: checked-in+
Attachment #508916 - Flags: checked-in+
Comment on attachment 508914 [details] [diff] [review]
Send constructors count to graph server

http://hg.mozilla.org/build/buildbotcustom/rev/93d1b3d2fda9

Backed out on default+production-0.8 branches due to tryserver bustage.  (self.graphBranch needs to be Tryserver, not MozillaTry)
Attachment #508914 - Flags: checked-in+ → checked-in-
Sorry :(

This wasn't caught in staging or in preproduction because we the use the same branch name for submission to graph server as we do for submission to tinderbox, and in both staging and preproduction the branch is hardcoded for everything to MozillaTest and Releng-Preproduction.

Not sure what the correct fix here is, there are a few options:
- Disable this for try like we do for leak test data and codesighs

- Specify that try's graph server branch isn't the same as its tinderbox branch...maybe something like graphBranch = config.get('graphs_branch', config['tinderbox_tree']), with corresponding changes to buildbot-configs
Blocks: 632399
Added support for optional graph_branch key in config, which will be set for try to Tryserver.

Move addBuildInfoSteps to where we're doing graph server posts. In particular, addLeakTestSteps and addBuildAnalysis steps don't do it all the time now. The  former is an optimization and the latter fixes busted XR builds.
Attachment #508914 - Attachment is obsolete: true
Attachment #510739 - Flags: review?(bhearsum)
Attachment #510739 - Flags: review?(bhearsum) → review+
Attachment #511480 - Flags: review?(bhearsum) → review+
Comment on attachment 510739 [details] [diff] [review]
Send constructors count to graph server

http://hg.mozilla.org/build/buildbotcustom/rev/e277edf1ae78
Attachment #510739 - Flags: checked-in+
Comment on attachment 511480 [details] [diff] [review]
Send try server results to Tryserver, not MozillaTry

http://hg.mozilla.org/build/buildbot-configs/rev/0749b4b1ad41
Attachment #511480 - Flags: checked-in+
Finally done!

Graphs can be seen at, e.g. http://graphs.mozilla.org/#tests=[[81,1,97]]

Do you want historical data inserted? I can import data for nightlies, it looks like this: http://graphs-stage.mozilla.org/graph.html#tests=[[80,6,8]]
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #31)
> Finally done!
> 
> Graphs can be seen at, e.g. http://graphs.mozilla.org/#tests=[[81,1,97]]

Awesome
 
> Do you want historical data inserted? I can import data for nightlies, it looks
> like this: http://graphs-stage.mozilla.org/graph.html#tests=[[80,6,8]]

The data before the 8th of December look very wrong.
> > Do you want historical data inserted? I can import data for nightlies, it looks
> > like this: http://graphs-stage.mozilla.org/graph.html#tests=[[80,6,8]]
> 
> The data before the 8th of December look very wrong.

Probably because of https://bugzilla.mozilla.org/attachment.cgi?id=489476 ?

If I take all that data, and divide by 8, subtract 2, will I get the correct results?
(In reply to comment #33)
> > > Do you want historical data inserted? I can import data for nightlies, it looks
> > > like this: http://graphs-stage.mozilla.org/graph.html#tests=[[80,6,8]]
> > 
> > The data before the 8th of December look very wrong.
> 
> Probably because of https://bugzilla.mozilla.org/attachment.cgi?id=489476 ?
> 
> If I take all that data, and divide by 8, subtract 2, will I get the correct
> results?

That'd be divided by 4 and substract 2, as it seems these numbers are from x86, not x64. And it looks like that would work, except for the value for commit 8e0fce7d5b49, which is really weird. Is it possible to find the corresponding tarball somewhere, even 6 months later ?
Waw, a static initializer in an idl.
by the way, this rules!
This broke SeaMonkey comm-1.9.1 linux nightlies. [I just deployed this change yesterday]

I landed a followup fix https://hg.mozilla.org/build/buildbotcustom/rev/250058e36789

We hit that only on 1.9.1 since there we don't use libxul, therefore libxul.so could not be found, and we hit that exception.
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.