Bug 211867 – [IWYU] Remove unnecessary includes from JSC implementation files

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ross Kirsling]

Continuation of bug 211821 -- let's go through include-what-you-use's recommended #include removals for Source/JavaScriptCore/**/*.cpp.

[Ross Kirsling]

Created attachment 399312 [details]
WIP Patch (directories L and up)

[Ross Kirsling]

Created attachment 399313 [details]
WIP Patch (directories L and up)

[Ross Kirsling]

Created attachment 399323 [details]
WIP Patch (directories L and up)

[Mark Lam]

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?

[Keith Miller]

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

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 399325 [details]
WIP Patch (directories L and up)

[Ross Kirsling]

Created attachment 399328 [details]
WIP Patch (directories L and up)

[Ross Kirsling]

Created attachment 399332 [details]
WIP Patch (directories L and up)

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 399436 [details]
Patch

[Ross Kirsling]

Created attachment 399439 [details]
Patch

[Ross Kirsling]

Created attachment 399441 [details]
Patch

[Ross Kirsling]

Created attachment 399446 [details]
Patch

[Ross Kirsling]

Created attachment 399447 [details]
Patch

[Ross Kirsling]

Created attachment 399449 [details]
Patch

[Ross Kirsling]

Created attachment 399450 [details]
Patch

[Ross Kirsling]

Comment on attachment 399450 [details]
Patch

Phew, I think all bots should be happy now.

[Ross Kirsling]

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

[Keith Miller]

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

[Joseph Pecoraro]

(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?

[Keith Miller]

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.

[Ross Kirsling]

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

[EWS]

Committed r261755: <https://trac.webkit.org/changeset/261755>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399450 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63284954>

[Ross Kirsling]

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.