Bug 232768 - Unify build of TestWebKitAPI/Tests/WebKitCocoa
Summary: Unify build of TestWebKitAPI/Tests/WebKitCocoa
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: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-05 15:12 PDT by Alex Christensen
Modified: 2021-11-09 18:27 PST (History)
5 users (show)

See Also:

Attachments
Patch (398.07 KB, patch)
2021-11-05 15:13 PDT, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (381.98 KB, patch)
2021-11-05 15:40 PDT, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (389.44 KB, patch)
2021-11-05 15:44 PDT, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (394.33 KB, patch)
2021-11-05 16:01 PDT, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (393.91 KB, patch)
2021-11-08 17:37 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (399.10 KB, patch)
2021-11-09 14:36 PST, Alex Christensen 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-11-05 15:12:56 PDT 
Unify build of TestWebKitAPI/Tests/WebKitCocoa
Comment 1 Alex Christensen 2021-11-05 15:13:20 PDT 
Created attachment 443441 [details]
Patch
Comment 2 Alex Christensen 2021-11-05 15:40:18 PDT 
Created attachment 443445 [details]
Patch
Comment 3 Alex Christensen 2021-11-05 15:44:49 PDT 
Created attachment 443447 [details]
Patch
Comment 4 Alex Christensen 2021-11-05 16:01:02 PDT 
Created attachment 443453 [details]
Patch
Comment 5 Alex Christensen 2021-11-05 16:14:39 PDT 
This reduces the time of "make -C Tools/TestWebKitAPI" from 86 seconds to 56 seconds
Comment 6 Alex Christensen 2021-11-08 17:37:05 PST 
Created attachment 443640 [details]
Patch
Comment 7 Tim Horton 2021-11-09 11:06:12 PST 
Comment on attachment 443640 [details]
Patch

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

> Tools/TestWebKitAPI/DeprecatedGlobalValues.h:33
> +extern bool receivedScriptMessage;

I do not love *this* whole thing, would be better to fix the tests to use local `__block bool`s like we often do, or maybe these names in a per-test namespace? As it stands this makes the tests oddly not self-contained. I guess it's not the worst thing, though.

> Tools/TestWebKitAPI/Scripts/generate-unified-sources.sh:18
> -UnifiedSourceMmFileCount=5
> +UnifiedSourceMmFileCount=50

Why so big, and why so big only for ObjC? This is the balance between fast incremental builds and fast world builds.
Comment 8 Alex Christensen 2021-11-09 11:09:29 PST 
Comment on attachment 443640 [details]
Patch

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

>> Tools/TestWebKitAPI/DeprecatedGlobalValues.h:33
>> +extern bool receivedScriptMessage;
> 
> I do not love *this* whole thing, would be better to fix the tests to use local `__block bool`s like we often do, or maybe these names in a per-test namespace? As it stands this makes the tests oddly not self-contained. I guess it's not the worst thing, though.

I don't love this whole thing either, but this gives us the benefits now.  I called it DeprecatedGlobalValues.h because we shouldn't increase the use of this.  We can go through and make all the tests only use local values and decrease the use of this file, but that's slow, low-priority work.

>> Tools/TestWebKitAPI/Scripts/generate-unified-sources.sh:18
>> +UnifiedSourceMmFileCount=50
> 
> Why so big, and why so big only for ObjC? This is the balance between fast incremental builds and fast world builds.

This patch only adds unified sources from TestWebKitAPI/Tests/WebKitCocoa which only contains .mm files, so that is the only limit that needs increasing in this patch.
This is a slight overshot, but not much.  It will make it so we don't need to increase this limit immediately after adding the next file.
Comment 9 Tim Horton 2021-11-09 12:26:07 PST 
(In reply to Alex Christensen from comment #8)
> Comment on attachment 443640 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443640&action=review
> 
> >> Tools/TestWebKitAPI/DeprecatedGlobalValues.h:33
> >> +extern bool receivedScriptMessage;
> > 
> > I do not love *this* whole thing, would be better to fix the tests to use local `__block bool`s like we often do, or maybe these names in a per-test namespace? As it stands this makes the tests oddly not self-contained. I guess it's not the worst thing, though.
> 
> I don't love this whole thing either, but this gives us the benefits now.  I
> called it DeprecatedGlobalValues.h because we shouldn't increase the use of
> this.  We can go through and make all the tests only use local values and
> decrease the use of this file, but that's slow, low-priority work.

Makes sense.

> >> Tools/TestWebKitAPI/Scripts/generate-unified-sources.sh:18
> >> +UnifiedSourceMmFileCount=50
> > 
> > Why so big, and why so big only for ObjC? This is the balance between fast incremental builds and fast world builds.
> 
> This patch only adds unified sources from TestWebKitAPI/Tests/WebKitCocoa
> which only contains .mm files, so that is the only limit that needs
> increasing in this patch.
> This is a slight overshot, but not much.  It will make it so we don't need
> to increase this limit immediately after adding the next file.

Oh oh oh wait, I misremembered; this isn't the "how many per bundle" count that I thought it was, this is just the total number of files. OK, fine.
Comment 10 Wenson Hsieh 2021-11-09 14:09:12 PST 
(In reply to Tim Horton from comment #9)
> (In reply to Alex Christensen from comment #8)
> > Comment on attachment 443640 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=443640&action=review
> > 
> > >> Tools/TestWebKitAPI/DeprecatedGlobalValues.h:33
> > >> +extern bool receivedScriptMessage;
> > > 
> > > I do not love *this* whole thing, would be better to fix the tests to use local `__block bool`s like we often do, or maybe these names in a per-test namespace? As it stands this makes the tests oddly not self-contained. I guess it's not the worst thing, though.
> > 
> > I don't love this whole thing either, but this gives us the benefits now.  I
> > called it DeprecatedGlobalValues.h because we shouldn't increase the use of
> > this.  We can go through and make all the tests only use local values and
> > decrease the use of this file, but that's slow, low-priority work.
> 
> Makes sense.

Yeah, I think this is a tiny bit unfortunate but (in practice) a non-issue since TestWebKitAPI is relaunched for each API test.

We should consider cleaning these global variables up to use local variables (or otherwise find a way to encapsulate this state per-test) so that we can parallelize API test execution in the future.

> 
> > >> Tools/TestWebKitAPI/Scripts/generate-unified-sources.sh:18
> > >> +UnifiedSourceMmFileCount=50
> > > 
> > > Why so big, and why so big only for ObjC? This is the balance between fast incremental builds and fast world builds.
> > 
> > This patch only adds unified sources from TestWebKitAPI/Tests/WebKitCocoa
> > which only contains .mm files, so that is the only limit that needs
> > increasing in this patch.
> > This is a slight overshot, but not much.  It will make it so we don't need
> > to increase this limit immediately after adding the next file.
> 
> Oh oh oh wait, I misremembered; this isn't the "how many per bundle" count
> that I thought it was, this is just the total number of files. OK, fine.
Comment 11 Alex Christensen 2021-11-09 14:36:56 PST 
Created attachment 443732 [details]
Patch
Comment 12 Alex Christensen 2021-11-09 17:05:36 PST 
http://trac.webkit.org/r285547
Comment 13 Radar WebKit Bug Importer 2021-11-09 17:06:33 PST 
<rdar://problem/85230604>