Keep track of .ctors to fight global initializers

RESOLVED FIXED

Status

P4
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: taras.mozilla, Assigned: catlee)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Updated

8 years ago
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.
(Reporter)

Updated

8 years ago
Depends on: 551531
Assignee: nobody → catlee
Priority: -- → P3
(Reporter)

Updated

8 years ago
Blocks: 569629
(Assignee)

Comment 3

8 years ago
Created attachment 460588 [details] [diff] [review]
Script to return the global constructor count on linux
Attachment #460588 - Flags: review?(bhearsum)
Attachment #460588 - Flags: feedback?(tglek)
(Assignee)

Comment 4

8 years ago
Created attachment 460589 [details] [diff] [review]
Count global constructors on linux
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-
(Assignee)

Comment 6

8 years ago
(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
(Reporter)

Comment 9

8 years ago
> 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?
(Assignee)

Comment 10

8 years ago
(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
(Assignee)

Comment 11

8 years ago
Created attachment 460660 [details] [diff] [review]
Count global constructors on linux
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+
(Assignee)

Comment 13

8 years ago
Comment on attachment 460588 [details] [diff] [review]
Script to return the global constructor count on linux

changeset:   709:6c1a3110a600
Attachment #460588 - Flags: checked-in+
(Assignee)

Comment 14

8 years ago
Comment on attachment 460660 [details] [diff] [review]
Count global constructors on linux

changeset:   846:8372cb6ea0d9
Attachment #460660 - Flags: checked-in+
(Assignee)

Updated

8 years ago
Depends on: 555205
Priority: P3 → P4
(Assignee)

Updated

8 years ago
Attachment #460588 - Flags: feedback?(tglek)
(Reporter)

Comment 15

8 years ago
is there an eta for getting warning emails on this?
(Assignee)

Comment 16

8 years ago
(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.
Created attachment 489476 [details] [diff] [review]
Enhance the script counting the number of ctors on linux

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
(Assignee)

Comment 21

8 years ago
Created attachment 508914 [details] [diff] [review]
Send constructors count to graph server
Attachment #508914 - Flags: review?(bhearsum)
(Assignee)

Comment 22

8 years ago
Created attachment 508916 [details] [diff] [review]
New test name on graph server

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+

Updated

8 years ago
Attachment #508916 - Flags: review?(bear) → review+
(Assignee)

Updated

8 years ago
Flags: needs-reconfig?
(Assignee)

Updated

8 years ago
Flags: needs-reconfig?
(Assignee)

Comment 23

8 years ago
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+
(Assignee)

Updated

8 years ago
Attachment #508916 - Flags: checked-in+

Comment 24

8 years ago
Merged and deployed in production:
http://hg.mozilla.org/build/buildbotcustom/rev/09e7dbe6c7b1

Comment 25

8 years ago
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-
(Assignee)

Comment 26

8 years ago
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

Updated

8 years ago
Blocks: 632399
(Assignee)

Comment 27

8 years ago
Created attachment 510739 [details] [diff] [review]
Send constructors count to graph server

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
(Assignee)

Updated

8 years ago
Attachment #510739 - Flags: review?(bhearsum)
Attachment #510739 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 28

8 years ago
Created attachment 511480 [details] [diff] [review]
Send try server results to Tryserver, not MozillaTry
Attachment #511480 - Flags: review?(bhearsum)
Attachment #511480 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 29

8 years ago
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+
(Assignee)

Comment 30

8 years ago
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+
(Assignee)

Comment 31

8 years ago
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
Last Resolved: 8 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.
(Assignee)

Comment 33

8 years ago
> > 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.
(Reporter)

Comment 40

8 years ago
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.