Bug 230198 – Postprocess framework headers in parallel

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Postprocess framework headers in parallel

[Tim Horton]

Created attachment 437983 [details]
Patch

[Tim Horton]

(if anybody knows how to easily limit the parallelism here, I'm all ears)

[Tim Horton]

(In reply to Tim Horton from comment #2)
> (if anybody knows how to easily limit the parallelism here, I'm all ears)

(at the same time, it doesn't really seem to be a problem?)

Comment on attachment 437983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437983&action=review

My understanding is that this script shouldn’t even get invoked when using Xcode’s modern build system. Are you fixing a problem seen when using the legacy build system?

> Source/WebKit/ChangeLog:20
> +        of time launching processes; it's likely possible to get further wins

Refactoring ${POSTPROCESS_HEADER_RULE}s to declare a function and using it from here would likely help more than this.

[Tim Horton]

(In reply to mitz from comment #4)
> Comment on attachment 437983 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437983&action=review
> 
> My understanding is that this script shouldn’t even get invoked when using
> Xcode’s modern build system. Are you fixing a problem seen when using the
> legacy build system?

Yes, we currently still use the legacy build system when building locally.

> > Source/WebKit/ChangeLog:20
> > +        of time launching processes; it's likely possible to get further wins
> 
> Refactoring ${POSTPROCESS_HEADER_RULE}s to declare a function and using it
> from here would likely help more than this.

Likely!

[Tim Horton]

Comment on attachment 437983 [details]
Patch

Let's see how this goes (we should see if we can improve more, though now WebKitAdditions rewriting is the hot path, not this).

[EWS]

Committed r282367 (241630@main): <https://commits.webkit.org/241630@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/83075272>

[Wenson Hsieh]

(In reply to Tim Horton from comment #6)
> Comment on attachment 437983 [details]
> Patch
> 
> Let's see how this goes (we should see if we can improve more, though now
> WebKitAdditions rewriting is the hot path, not this).

(Assuming you're referring to replace-webkit-additions-includes.py, and the build phase that calls into it) I think we should actually be able to remove that build phase and script altogether, since we don't currently have any public APIs whose name cannot be landed in open source.

I think that if we need it again in the future, we can just resurrect it (since it exists in git history anyways).

[Tim Horton]

(In reply to Wenson Hsieh from comment #9)
> (In reply to Tim Horton from comment #6)
> > Comment on attachment 437983 [details]
> > Patch
> > 
> > Let's see how this goes (we should see if we can improve more, though now
> > WebKitAdditions rewriting is the hot path, not this).
> 
> (Assuming you're referring to replace-webkit-additions-includes.py, and the
> build phase that calls into it) I think we should actually be able to remove
> that build phase and script altogether, since we don't currently have any
> public APIs whose name cannot be landed in open source.
> 
> I think that if we need it again in the future, we can just resurrect it
> (since it exists in git history anyways).

Good call, that's another 4 seconds (down to 11!)

[Elliott Williams]

Comment on attachment 437983 [details]
Patch

The good news is that XCBuild doesn't use this script at all: It runs the postprocessing step on each header in parellel. But this seems fine to me as a temporary stopgap :)

[Elliott Williams]

(In reply to Elliott Williams from comment #11)
> Comment on attachment 437983 [details]
> Patch
> 
> The good news is that XCBuild doesn't use this script at all: It runs the
> postprocessing step on each header in parellel. But this seems fine to me as
> a temporary stopgap :)

Er, actually, we've already deleted this file as part of the XCBuild migration -- this headers phase is one of the parts of the build that has adopted XCBuild early.

[Tim Horton]

(In reply to Elliott Williams from comment #12)
> (In reply to Elliott Williams from comment #11)
> > Comment on attachment 437983 [details]
> > Patch
> > 
> > The good news is that XCBuild doesn't use this script at all: It runs the
> > postprocessing step on each header in parellel. But this seems fine to me as
> > a temporary stopgap :)
> 
> Er, actually, we've already deleted this file as part of the XCBuild
> migration -- this headers phase is one of the parts of the build that has
> adopted XCBuild early.

This landed a long time ago!