Bug 224989 – Make TestInput immutable

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sam Sneddon [:gsnedders]]

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.

[Sam Sneddon [:gsnedders]]

Created attachment 426935 [details]
Patch

[Jonathan Bedard]

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

[Sam Sneddon [:gsnedders]]

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

[Sam Sneddon [:gsnedders]]

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)

[Sam Sneddon [:gsnedders]]

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

[Sam Sneddon [:gsnedders]]

Created attachment 427174 [details]
Patch

[Sam Sneddon [:gsnedders]]

With the new patch we're back to the performance of the main branch.

[Jonathan Bedard]

Comment on attachment 427174 [details]
Patch

Lets wait for iOS results before landing, but I'm happy with this change

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/77232886>