977338 - Drop chunk size of unified sources in js/src

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Steve Fink [:sfink] [:s:]]

They take too long to compile. I want incremental builds often.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Smaller unified chunks for js/src

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.

Comment 2

[Mike Hommey [:glandium]]

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)

Comment 3

[Steve Fink [:sfink] [:s:]]

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.)

Comment 4

[Michael Shal [:mshal]]

Can you show what the changes to incremental build times are as well? (Maybe cherry-pick a few .cpp files, or something)

Comment 5

[Terrence Cole [:terrence]]

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.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Remove AtomicOperations-inl.h

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.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Remove AtomicOperations-inl.h

It hit some other unified build problems. This one probably won't work either, but it's closer.

Comment 8

[Lars T Hansen [:lth]]

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

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Remove AtomicOperations-inl.h

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eda99e9e730d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b20eccdb23e

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eda99e9e730d
https://hg.mozilla.org/mozilla-central/rev/2b20eccdb23e