Continuation of bug 211821 -- let's go through include-what-you-use's recommended #include removals for Source/JavaScriptCore/**/*.cpp.
Created attachment 399312 [details] WIP Patch (directories L and up)
Created attachment 399313 [details] WIP Patch (directories L and up)
Created attachment 399323 [details] WIP Patch (directories L and up)
What is your methodology for doing this change? (1) Just trial and error to see if removing a #include causes a compilation error? Or (2)do you have another means of doing this?
(In reply to Mark Lam from comment #4) > What is your methodology for doing this change? (1) Just trial and error to > see if removing a #include causes a compilation error? Or (2)do you have > another means of doing this? [IWYU] is "include what you use" it's a clang tool. Although, it seems to have broken things here...
(In reply to Mark Lam from comment #4) > What is your methodology for doing this change? (1) Just trial and error to > see if removing a #include causes a compilation error? Or (2)do you have > another means of doing this? I described it in bug 211821 comment 2 -- IWYU spits out a list of recommended insertions/deletions and I'm going through them manually. It's tedious, but more sustainable than blind trial-and-error...although given how error-prone it is, the cost/benefit might be questionable. I think it gets pretty confused by our *Inlines.h approach; evidently there are lots of linking errors that simply won't happen with JSCOnly.
Created attachment 399325 [details] WIP Patch (directories L and up)
Created attachment 399328 [details] WIP Patch (directories L and up)
Created attachment 399332 [details] WIP Patch (directories L and up)
(Also, sorry for the noise; EWS auto-CCs even without r? but I'm planning to finish the other half of this before asking for review...)
Created attachment 399436 [details] Patch
Created attachment 399439 [details] Patch
Created attachment 399441 [details] Patch
Created attachment 399446 [details] Patch
Created attachment 399447 [details] Patch
Created attachment 399449 [details] Patch
Created attachment 399450 [details] Patch
Comment on attachment 399450 [details] Patch Phew, I think all bots should be happy now.
To be slightly more explicit about how I "went through the removals manually", for the curious: 1. Open a batch of files and prune includes from each as recommended 2. Build and see what compile/link errors occur a. First do the non-unified JSCOnly build as usual. b. Then do a normal build-jsc run, since it encounters WAY more link errors (in spite of being a unified build, which ideally shouldn't be used here at all). 3. Put some includes back a. Compile errors are usually as simple as "you said we didn't need that line but clearly we do". b. Link errors are always because of an *Inlines.h include. In most of these cases, it's possible to replace that include with a single smaller *Inlines.h (but if you need multiple then you may as well restore the original). 4. Profit? I wouldn't necessarily recommend others to spend multiple days of their life doing such a thing, but as you can see, the manual effort does produce concrete results, whereas I probably would've just given up outright before ever figuring out how to make IWYU play nicely with WK in an automated fashion (as KeithR said he did a number of years ago).
(In reply to Ross Kirsling from comment #19) > To be slightly more explicit about how I "went through the removals > manually", for the curious: > > 1. Open a batch of files and prune includes from each as recommended > > 2. Build and see what compile/link errors occur > a. First do the non-unified JSCOnly build as usual. > b. Then do a normal build-jsc run, since it encounters WAY more link > errors (in spite of being a unified build, which ideally shouldn't be used > here at all). > > 3. Put some includes back > a. Compile errors are usually as simple as "you said we didn't need that > line but clearly we do". > b. Link errors are always because of an *Inlines.h include. In most of > these cases, it's possible to replace that include with a single smaller > *Inlines.h (but if you need multiple then you may as well restore the > original). > > 4. Profit? > > I wouldn't necessarily recommend others to spend multiple days of their life > doing such a thing, but as you can see, the manual effort does produce > concrete results, whereas I probably would've just given up outright before > ever figuring out how to make IWYU play nicely with WK in an automated > fashion (as KeithR said he did a number of years ago). I'm kinda surprised that IWYU can't figure out how to sink the includes. I'm guessing it's because it doesn't look at every translation unit at once? Seems like it could export some graph to be processed offline though. Not saying that's a trivial project...
(In reply to Ross Kirsling from comment #18) > Comment on attachment 399450 [details] > Patch > > Phew, I think all bots should be happy now. Wow, nice! Did this produce any noticeable build time improvement?
Comment on attachment 399450 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399450&action=review rs=me > Source/JavaScriptCore/ChangeLog:9 > + * API/: > + * assembler/: Lol, I love the blank lines.
(In reply to Joseph Pecoraro from comment #21) > (In reply to Ross Kirsling from comment #18) > > Comment on attachment 399450 [details] > > Patch > > > > Phew, I think all bots should be happy now. > > Wow, nice! Did this produce any noticeable build time improvement? I'm hoping so! Gonna do a before-and-after now and see what time/size differences I can detect.
Committed r261755: <https://trac.webkit.org/changeset/261755> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399450 [details].
<rdar://problem/63284954>
Well, it doesn't seem like there's any difference in the size of the build products with a normal unified `build-jsc` run (which I guess shouldn't surprise me). My Debug build did complete 45s faster, although I don't have enough data points to ensure this wasn't a fluke.