Bug 230198 - Postprocess framework headers in parallel
Summary: Postprocess framework headers in parallel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-12 02:47 PDT by Tim Horton
Modified: 2022-04-15 15:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.97 KB, patch)
2021-09-12 02:50 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2021-09-12 02:47:09 PDT
Postprocess framework headers in parallel
Comment 1 Tim Horton 2021-09-12 02:50:30 PDT
Created attachment 437983 [details]
Patch
Comment 2 Tim Horton 2021-09-12 02:52:53 PDT
(if anybody knows how to easily limit the parallelism here, I'm all ears)
Comment 3 Tim Horton 2021-09-12 02:54:24 PDT
(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 4 mitz 2021-09-12 07:59:22 PDT
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.
Comment 5 Tim Horton 2021-09-12 20:59:56 PDT
(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!
Comment 6 Tim Horton 2021-09-13 15:13:15 PDT
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).
Comment 7 EWS 2021-09-13 15:26:27 PDT
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].
Comment 8 Radar WebKit Bug Importer 2021-09-13 15:27:32 PDT
<rdar://problem/83075272>
Comment 9 Wenson Hsieh 2021-09-13 18:38:22 PDT
(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).
Comment 10 Tim Horton 2021-09-15 01:50:21 PDT
(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!)
Comment 11 Elliott Williams 2022-04-15 15:07:08 PDT
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 :)
Comment 12 Elliott Williams 2022-04-15 15:09:23 PDT
(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.
Comment 13 Tim Horton 2022-04-15 15:35:21 PDT
(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!