JetStream2 driver should be able to specify individual subtests/test groups to run #6046

xw · 2022-11-02T20:07:04Z

`c770d8f`

JetStream2 driver should be able to specify individual subtests/test groups to run
https://bugs.webkit.org/show_bug.cgi?id=247301
rdar://101783726

Reviewed by Alexey Shvayka.

- Refactor existing `report` argument parsing to use URLSearchParams
- Add a new URL parameter, `test`, that runs a given list of tests and/or test groups.

Eventually, we'd like to be able to add profiling/ARTrace support to JetStream2, and being able to measure individual subtests would be useful as measuring the whole test may be too long and costly.

* PerformanceTests/JetStream2/JetStreamDriver.js:
(Driver.prototype.async initialize):
(Driver.prototype.async reportScoreToRunBenchmarkRunner):
(prototype.else):
* PerformanceTests/JetStream3/JetStreamDriver.js:
(Driver.prototype.async initialize):
(Driver.prototype.async reportScoreToRunBenchmarkRunner):
(prototype.else):

Canonical link: https://commits.webkit.org/256264@main

3cca236

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe
	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 api-mac~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
✅ 🛠 🧪 merge	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	~~🧪 mac-wk2-stress~~

webkit-early-warning-system · 2022-11-02T20:08:08Z

EWS run on previous version of this PR (hash d52e2c0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	~~🧪 api-mac~~	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

shvaikalesh · 2022-11-02T23:47:59Z

PerformanceTests/JetStream2/JetStreamDriver.js

@@ -494,7 +496,7 @@ class Driver {
        if (!isInBrowser)
            return;

-        if (window.location.search !== '?report=true')
+        if (!urlParameters.get('report'))


nit:

Suggested change

if (!urlParameters.get('report'))

if (!urlParameters.has('report'))

Hmm, this will not work as expected if I have ?report=false.

webkit-early-warning-system · 2022-11-02T23:57:11Z

EWS run on previous version of this PR (hash 7513042)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-03T00:01:04Z

EWS run on previous version of this PR (hash 8570d3d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
	⏳ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	✅ 🛠 tv	⏳ 🧪 mac-wk1
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-03T00:05:41Z

EWS run on previous version of this PR (hash 9848c27)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	✅ 🛠 tv	⏳ 🧪 mac-wk1
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

dewei-zhu · 2022-11-03T00:11:53Z

PerformanceTests/JetStream2/JetStreamDriver.js

@@ -494,7 +496,7 @@ class Driver {
        if (!isInBrowser)
            return;

-        if (window.location.search !== '?report=true')
+        if (!urlParameters.has('report'))


I think we should keep using urlParameters.get('report') as ?report=false is a valid case and in that case we want the early return in next line.

Also, urlParameters.get('report') gives "false" string, which will be evaluated true.
If we only allow report key to have "true" or "false" as input, then, we should do
if (urlParameters.get('report') != 'true') instead.

dewei-zhu · 2022-11-03T00:12:04Z

PerformanceTests/JetStream3/JetStreamDriver.js

@@ -494,7 +496,7 @@ class Driver {
        if (!isInBrowser)
            return;

-        if (window.location.search !== '?report=true')
+        if (!urlParameters.has('report'))


dewei-zhu · 2022-11-03T00:27:32Z

PerformanceTests/JetStream2/JetStreamDriver.js

@@ -494,7 +496,7 @@ class Driver {
        if (!isInBrowser)
            return;

-        if (window.location.search !== '?report=true')
+        if (!urlParameters.has('report'))


Also, urlParameters.get('report') gives "false" string, which will be evaluated true.
If we only allow report key to have "true" or "false" as input, then, we should do
if (urlParameters.get('report') != 'true') instead.

dewei-zhu · 2022-11-03T00:28:43Z

PerformanceTests/JetStream2/JetStreamDriver.js

@@ -404,7 +406,7 @@ class Driver {
        await this.prefetchResourcesForBrowser();
        await this.fetchResources();
        this.prepareToRun();
-        if (isInBrowser && window.location.search == '?report=true') {
+        if (isInBrowser && urlParameters.get('report')) {


Same condition check issue mentioned below.

webkit-early-warning-system · 2022-11-03T00:35:38Z

EWS run on current version of this PR (hash 3cca236)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe
	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 api-mac~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk1~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
✅ 🛠 🧪 merge	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	~~🧪 mac-wk2-stress~~

…groups to run https://bugs.webkit.org/show_bug.cgi?id=247301 rdar://101783726 Reviewed by Alexey Shvayka. - Refactor existing `report` argument parsing to use URLSearchParams - Add a new URL parameter, `test`, that runs a given list of tests and/or test groups. Eventually, we'd like to be able to add profiling/ARTrace support to JetStream2, and being able to measure individual subtests would be useful as measuring the whole test may be too long and costly. * PerformanceTests/JetStream2/JetStreamDriver.js: (Driver.prototype.async initialize): (Driver.prototype.async reportScoreToRunBenchmarkRunner): (prototype.else): * PerformanceTests/JetStream3/JetStreamDriver.js: (Driver.prototype.async initialize): (Driver.prototype.async reportScoreToRunBenchmarkRunner): (prototype.else): Canonical link: https://commits.webkit.org/256264@main

webkit-commit-queue · 2022-11-03T01:46:42Z

Committed 256264@main (c770d8f): https://commits.webkit.org/256264@main

Reviewed commits have been landed. Closing PR #6046 and removing active labels.

mikhailramalho · 2022-11-03T21:44:49Z

hi @xw, this patch broke JetStream2 on linux (tested on 32 and 64 bits):

$ ~/tools/WebKit/WebKitBuild/Release/bin/jsc watch-cli.js 
Exception: ReferenceError: Can't find variable: URLSearchParams
global code@/home/mgadelha/tools/WebKit/PerformanceTests/JetStream2/JetStreamDriver.js:45:42
load@[native code]
global code@/home/mgadelha/tools/WebKit/PerformanceTests/JetStream2/cli.js:46:5
load@[native code]
global code@watch-cli.js:65:5

xw · 2022-11-03T22:20:41Z

Looks like URLSearchParams exists only in the browser and not jsc. These arguments aren't relevant when running tests inside the jsc shell, so it should be fine to disable in that case.

Fix for this incoming shortly.

xw · 2022-11-03T23:14:05Z

#6109

xw self-assigned this Nov 2, 2022

xw added Other Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases labels Nov 2, 2022

xw requested a review from Constellation November 2, 2022 20:08

xw requested review from MenloDorian, saambarati, sundiamonde and dewei-zhu November 2, 2022 20:08

dewei-zhu requested a review from justinmichaud November 2, 2022 21:35

shvaikalesh approved these changes Nov 2, 2022

View reviewed changes

dewei-zhu reviewed Nov 3, 2022

View reviewed changes

xw added the merge-queue Applied to send a pull request to merge-queue label Nov 3, 2022

webkit-early-warning-system merged commit c770d8f into WebKit:main Nov 3, 2022

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JetStream2 driver should be able to specify individual subtests/test groups to run #6046

JetStream2 driver should be able to specify individual subtests/test groups to run #6046

xw commented Nov 2, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 2, 2022 •

edited

shvaikalesh Nov 2, 2022

dewei-zhu Nov 3, 2022

webkit-early-warning-system commented Nov 2, 2022 •

edited

webkit-early-warning-system commented Nov 3, 2022 •

edited

webkit-early-warning-system commented Nov 3, 2022 •

edited

dewei-zhu Nov 3, 2022

dewei-zhu Nov 3, 2022

dewei-zhu Nov 3, 2022

dewei-zhu Nov 3, 2022

dewei-zhu Nov 3, 2022

webkit-early-warning-system commented Nov 3, 2022 •

edited

webkit-commit-queue commented Nov 3, 2022

mikhailramalho commented Nov 3, 2022

xw commented Nov 3, 2022

xw commented Nov 3, 2022

	if (!urlParameters.get('report'))
	if (!urlParameters.has('report'))

JetStream2 driver should be able to specify individual subtests/test groups to run #6046

JetStream2 driver should be able to specify individual subtests/test groups to run #6046

Conversation

xw commented Nov 2, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 2, 2022 • edited

webkit-early-warning-system commented Nov 3, 2022 • edited

webkit-early-warning-system commented Nov 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 3, 2022 • edited

webkit-commit-queue commented Nov 3, 2022

mikhailramalho commented Nov 3, 2022

xw commented Nov 3, 2022

xw commented Nov 3, 2022

xw commented Nov 2, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 2, 2022 •

edited

webkit-early-warning-system commented Nov 2, 2022 •

edited

webkit-early-warning-system commented Nov 3, 2022 •

edited

webkit-early-warning-system commented Nov 3, 2022 •

edited

webkit-early-warning-system commented Nov 3, 2022 •

edited