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.
Created attachment 426935 [details] Patch
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
(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.
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)
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
Created attachment 427174 [details] Patch
With the new patch we're back to the performance of the main branch.
Comment on attachment 427174 [details] Patch Lets wait for iOS results before landing, but I'm happy with this change
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].
<rdar://problem/77232886>