Bug 232768 – Unify build of TestWebKitAPI/Tests/WebKitCocoa

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

[Alex Christensen]

Unify build of TestWebKitAPI/Tests/WebKitCocoa

[Alex Christensen]

Created attachment 443441 [details]
Patch

[Alex Christensen]

Created attachment 443445 [details]
Patch

[Alex Christensen]

Created attachment 443447 [details]
Patch

[Alex Christensen]

Created attachment 443453 [details]
Patch

[Alex Christensen]

This reduces the time of "make -C Tools/TestWebKitAPI" from 86 seconds to 56 seconds

[Alex Christensen]

Created attachment 443640 [details]
Patch

[Tim Horton]

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.

[Alex Christensen]

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.

[Tim Horton]

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

[Wenson Hsieh]

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

[Alex Christensen]

Created attachment 443732 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r285547

[Radar WebKit Bug Importer]

<rdar://problem/85230604>