Bug 224989

Summary: Make TestInput immutable
Product: WebKit Reporter: Sam Sneddon [:gsnedders] <gsnedders>
Component: Tools / TestsAssignee: Sam Sneddon [:gsnedders] <gsnedders>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, glenn, jbedard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=225115
Attachments:
Description Flags
Patch
none
Patch none

Description Sam Sneddon [:gsnedders] 2021-04-23 12:31:47 PDT
The main point here is moving computing reference_files and should_run_pixel_test to when we initially construct TestInput, as at this point this happens in the some process and thread (since bug 221577), hence there's no real reason for it to happen later. With this done, it should then be possible to make TestInput immutable, which should help make things easier to understand.
Comment 1 Sam Sneddon [:gsnedders] 2021-04-23 12:38:07 PDT
Created attachment 426935 [details]
Patch
Comment 2 Jonathan Bedard 2021-04-26 16:22:26 PDT
Comment on attachment 426935 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/models/test_input.py:-48
> -        # because they require us to look at the filesystem and we want to be able to do that in parallel.

I was curious if this comment was actually true and makes a difference, and it looks like make a pretty large difference. I tested this with the following command:
    `time run-webkit-tests --root ../buildToTest --dry-run fast`
Without the patch, this takes 27 seconds, with the patch, 56 seconds. I think things comes out to being about a minute with all 60 tests, but it's hard to tell because starting the web servers takes so much time. I think we need to construct the TestInput objects in parallel in some capacity
Comment 3 Sam Sneddon [:gsnedders] 2021-04-26 23:40:30 PDT
(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 426935 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426935&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_input.py:-48
> > -        # because they require us to look at the filesystem and we want to be able to do that in parallel.
> 
> I was curious if this comment was actually true and makes a difference, and
> it looks like make a pretty large difference. I tested this with the
> following command:
>     `time run-webkit-tests --root ../buildToTest --dry-run fast`
> Without the patch, this takes 27 seconds, with the patch, 56 seconds. I
> think things comes out to being about a minute with all 60 tests, but it's
> hard to tell because starting the web servers takes so much time. I think we
> need to construct the TestInput objects in parallel in some capacity

Will investigate the perf regression later today, but note that that comment has been wrong since bug 221577.
Comment 4 Sam Sneddon [:gsnedders] 2021-04-27 06:39:01 PDT
Oh, but of course, we generate all the TestInputs twice!

> reference_files (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/port/base.py:510)
> _test_input_for_file (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:218)
> _get_test_inputs (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:266)
> _update_worker_count (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:270)
> _set_up_run (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:281)
> run (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:385)

and

> reference_files (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/port/base.py:510)
> _test_input_for_file (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:218)
> _get_test_inputs (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:266)
> _run_tests (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:533)
> _run_test_subset (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:465)
> run (/Volumes/gsnedders/projects/Safari/OpenSource/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:391)
Comment 5 Sam Sneddon [:gsnedders] 2021-04-27 07:24:24 PDT
For reference, on main, redundantly creating all the TestInputs twice is only about 1% of the cost of the Manager process.

Some rough measurements for the above command:

r272567 (i.e., pre bug 221577): 9.5s
main: 22.1s
main+patch: 33.7s
Comment 6 Sam Sneddon [:gsnedders] 2021-04-27 11:19:12 PDT
Created attachment 427174 [details]
Patch
Comment 7 Sam Sneddon [:gsnedders] 2021-04-27 11:30:21 PDT
With the new patch we're back to the performance of the main branch.
Comment 8 Jonathan Bedard 2021-04-27 11:49:55 PDT
Comment on attachment 427174 [details]
Patch

Lets wait for iOS results before landing, but I'm happy with this change
Comment 9 EWS 2021-04-27 15:09:52 PDT
Committed r276670 (237089@main): <https://commits.webkit.org/237089@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427174 [details].
Comment 10 Radar WebKit Bug Importer 2021-04-27 15:10:23 PDT
<rdar://problem/77232886>