Drop chunk size of unified sources in js/src

RESOLVED FIXED in Firefox 44

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla44
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

They take too long to compile. I want incremental builds often.
I am really not sure who to ask for review on this.

I haven't done any timings to see how much this hurts full build times, which I probably ought to do in order to pick the right size. But JS devs do lots of single .cpp file rebuilds, so the chunk size should definitely be much smaller than 16.
Attachment #8486112 - Flags: review?(mh+mozilla)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8486112 [details] [diff] [review]
Smaller unified chunks for js/src

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

This is the right way to do it, but you should seek review of the value itself by the most concerned people, so someone from the js team would be better to review this.

(Also, yes, please do get timings for full tree builds with different values ; if you do that on try, don't forget to disable sccache)
Attachment #8486112 - Flags: review?(mh+mozilla) → feedback+
Timings for the full tree builds:

16 - 6m11s
16 - 6m11s
 6 - 6m51s
 4 - 7m20s
 2 - 8m39s

I still need to do timings for single-file changes. But so far, 6 is looking pretty good to me. (Not sure why I didn't try 8.)
Can you show what the changes to incremental build times are as well? (Maybe cherry-pick a few .cpp files, or something)
Depends on: 1203297
Comment on attachment 8486112 [details] [diff] [review]
Smaller unified chunks for js/src

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

Let's do it. We can always quibble over the exact number later, but 6 seems reasonable to me -- seems to increase my clean rebuild time by <50% but lower my single file rebuild from 20s to 2s.
Attachment #8486112 - Flags: review+
Posted patch Remove AtomicOperations-inl.h (obsolete) — Splinter Review
I know there was some discussion on IRC today about AtomicOperations-inl.h breaking the unified build, and in fact it seems like a fix was checked in. I would like to drop the unified chunk size from 16 to 6 within js/src, but doing so triggers AtomicOperations-inl.h to break the build again -- there's a compilation unit that wants memcpySafeWhenRacy, but it's not actually emitted into *any* object file, so the link fails.

I probably should have read through what other people were discussing, but when I looked at it, I didn't understand why a -inl.h file is needed here in the first place. I merged it into AtomicOperations.h and my build is now happy.

I imagine there's a good reason for it to exist, and I'll probably discover it with the try push I'm about to make. But in the off chance that I am right, I'll post this for review prematurely. You'll probably want to check for the expected wreckage at https://treeherder.mozilla.org/#/jobs?repo=try&revision=75bde466c146 before bothering to read the patch.
Attachment #8670100 - Flags: review?(lhansen)
Posted patch Remove AtomicOperations-inl.h (obsolete) — Splinter Review
It hit some other unified build problems. This one probably won't work either, but it's closer.
Attachment #8670114 - Flags: review?(lhansen)
Attachment #8670100 - Attachment is obsolete: true
Attachment #8670100 - Flags: review?(lhansen)
(In reply to Steve Fink [:sfink, :s:] from comment #7)
> Created attachment 8670114 [details] [diff] [review]
> Remove AtomicOperations-inl.h
> 
> It hit some other unified build problems. This one probably won't work
> either, but it's closer.

ISTR the reason that file is an -inl.h file is to avoid some dependencies that appear when it is a .h file.  Those may have been historical, and I don't remember exactly what they were, but I would not be surprised if there are some problems.

(I still think the real bug is TypedArrayCommon.h, not this file.  Having or not having an -inl.h file should not be breaking the build.  Every cpp file that needs it should be including the -inl.h file, concatenating those files should not change that.)
I compiled this locally for the 32-bit ARM simulator with FILES_PER_UNIFIED_FILE=1 and it was happy. The latest try push is https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcf5c931dce which so far only shows seemingly unrelated errors. And I'm afraid it will bitrot quickly.
Attachment #8670319 - Flags: review?(lhansen)
Attachment #8670114 - Attachment is obsolete: true
Attachment #8670114 - Flags: review?(lhansen)
Attachment #8670319 - Flags: review?(lhansen) → review+
https://hg.mozilla.org/mozilla-central/rev/eda99e9e730d
https://hg.mozilla.org/mozilla-central/rev/2b20eccdb23e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.