WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
218296
[GPUProcess] Override GPUProcess defaults for API testing
https://bugs.webkit.org/show_bug.cgi?id=218296
Summary
[GPUProcess] Override GPUProcess defaults for API testing
Jonathan Bedard
Reported
2020-10-28 11:03:39 PDT
There are a few reasons we may not want to do this, but I want the change up so it can be discussed, at least.
Attachments
Patch
(6.72 KB, patch)
2020-10-28 11:23 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(16.55 KB, patch)
2020-11-06 11:34 PST
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(16.55 KB, patch)
2020-11-06 13:06 PST
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch
(13.80 KB, patch)
2020-11-06 16:21 PST
,
Jonathan Bedard
jbedard
: review?
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2020-10-28 11:03:58 PDT
<
rdar://problem/70771368
>
Jonathan Bedard
Comment 2
2020-10-28 11:23:38 PDT
Created
attachment 412548
[details]
Patch
Sam Weinig
Comment 3
2020-10-28 12:43:38 PDT
I am not sure this will do what you want in all cases. Default values in WebPreferencesDefaultValues.cpp are expected to be consistent for all environments, meaning they shouldn't depend on external factors that might be different depending on which process they are run in. This means you probably don't want to use environment variables, as they won't necessarily have the same value in the UIProcess and WebProcess.
Sam Weinig
Comment 4
2020-10-28 12:52:35 PDT
An alternative strategy you could employ if you want this result would be to move the preferences you would like an alternative default for to WebPreferencesDebug.yaml (really, we should merge WebPreferencesDebug.yaml and WebPreferencesInternal.yaml and make this true for all internal features), and then, at least on cocoa platforms, you can use [[NSUserDefaults standardUserDefaults] setObject:@YES forKey:@"WebKit2.<key name here>"] (I think that is the syntax, it has been a while).
Darin Adler
Comment 5
2020-10-28 14:00:29 PDT
I think Sam’s right that environment variables aren’t going to make their way across all the processes they way you’d want. We definitely want to do this with some feature of the "settings for testing" machinery, not with another mechanism (environment variables). It seems OK to me to have it be a command line option various test runners.
Jonathan Bedard
Comment 6
2020-10-28 14:46:50 PDT
The central issue here is that API tests don't have the machinery to turn features on and off because each individual test sets specific features from the WebKit defaults. One of the reasons I didn't mark the original patch for review is because the fact that API tests work this way make it unclear that allowing contributors to enable a feature via the test harness is even correct.
Darin Adler
Comment 7
2020-10-28 14:50:44 PDT
(In reply to Jonathan Bedard from
comment #6
)
> The central issue here is that API tests don't have the machinery to turn > features on and off because each individual test sets specific features from > the WebKit defaults.
You left out one key word here. Each individual test *can* set specific features from WebKit defaults. Many do not.
> One of the reasons I didn't mark the original patch for > review is because the fact that API tests work this way make it unclear that > allowing contributors to enable a feature via the test harness is even > correct.
I don’t agree with this line of thinking. Preferences have default values. Tests can override those defaults to turn features on for testing, even when they are not yet on across the board. And I think we can let command line arguments get in between those two levels, overriding the default value, but still allowing the test to override what the command line said.
Jonathan Bedard
Comment 8
2020-10-28 15:00:11 PDT
(In reply to Darin Adler from
comment #7
)
> (In reply to Jonathan Bedard from
comment #6
) > > The central issue here is that API tests don't have the machinery to turn > > features on and off because each individual test sets specific features from > > the WebKit defaults. > > You left out one key word here. > > Each individual test *can* set specific features from WebKit defaults. Many > do not. > > > One of the reasons I didn't mark the original patch for > > review is because the fact that API tests work this way make it unclear that > > allowing contributors to enable a feature via the test harness is even > > correct. > > I don’t agree with this line of thinking. > > Preferences have default values. Tests can override those defaults to turn > features on for testing, even when they are not yet on across the board. And > I think we can let command line arguments get in between those two levels, > overriding the default value, but still allowing the test to override what > the command line said.
We could let command line arguments override default values, yes, it's just not obvious that supporting this for API tests is valuable. Whatever we do, it should be something inside WebKit, unless we want the test runner to have a shared generator that gives API tests a different set of defaults than WebKit (my understanding is that such a system is very undesirable for the way API tests are written and used). The UserDefaults approach does seem better than the environment variable approach, I'm still not convinced we want even that.
Darin Adler
Comment 9
2020-10-28 15:02:19 PDT
I am really confused about the uncertainty here. Of course we must have a way to turn on new features so we can test them. Are you saying that you’re thinking API tests should sick with a way for *tests* a way to turn them on so that the *command line* does not need to?
Tim Horton
Comment 10
2020-10-28 15:08:20 PDT
(Having been privy to some of the other discussions about this). Some of the confusion comes from the fact that this hasn't been done before; there's no precedent (as far as any of us have found) for having globally-adjustable features enabled globally in API tests (we were all a bit surprised). So the fact that we've managed large architectural off-by-default features without this before (enabling it in tests for the feature itself) leads to the "well why didn't we need it before?!" question (say, for accelerated drawing, or UI-side compositing, or PSON). I think the WebPreferences-based approach is probably a decent one, though, and sufficiently noninvasive to be worth it (especially if run-api-tests gains a command line param to set an arbitrary set of WebPreferencesDebug-backed defaults).
Jonathan Bedard
Comment 11
2020-10-28 15:17:50 PDT
(In reply to Darin Adler from
comment #9
)
> I am really confused about the uncertainty here. > > Of course we must have a way to turn on new features so we can test them. > > Are you saying that you’re thinking API tests should sick with a way for > *tests* a way to turn them on so that the *command line* does not need to?
This has essentially been the logic API tests have used thus far (that is, if you would like to test a specific feature, write a new test for that feature, rather than running all existing tests for the new feature, like we would with layout tests) If we're saying we want a way to change which features are enabled or disabled by default, as a general statement (that is, not just for the GPU process), we either need something like isFeatureFlagEnabled(...) that pulls from a file or UserDefaults or we need a kind of configuration generator in the API testing binary that can set a different set of defaults.
Jonathan Bedard
Comment 12
2020-11-06 11:34:07 PST
Created
attachment 413453
[details]
Patch
Jonathan Bedard
Comment 13
2020-11-06 12:02:53 PST
(In reply to Jonathan Bedard from
comment #12
)
> Created
attachment 413453
[details]
> Patch
New patch uses `defaults write` in run-api-tests, which I think is cleaner than trying to strip arguments out of argv before passing them to gtest. We're obviously enabling GPU Process for any API tests running in parallel with us, but API tests already break if you run them that way.
Darin Adler
Comment 14
2020-11-06 12:08:11 PST
Does this mean that if you hit Control-C part-way through, you end up in GPU Process mode? If so, is that limited to the API tests binary, without affecting Safari, MiniBrowser, WebKitTestRunner, or DumpRenderTree?
Ryan Haddad
Comment 15
2020-11-06 12:14:19 PST
Comment on
attachment 413453
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=413453&action=review
> Tools/Scripts/webkitpy/api_tests/run_api_tests.py:144 > + help='Enable the GPU process through defaults write'),
I think this help test was supposed to be "Disable the GPU process..."
Sam Weinig
Comment 16
2020-11-06 12:19:04 PST
Comment on
attachment 413453
[details]
Patch I think rather than using 'defaults write' externally and persistently, you want to use one of the volatile domains. I can't recall how we made TestWebKitAPI, so it may be possible to use the NSArgumentDomain and just pass the defaults directly on the command line, or you may need to modify the test runner binary itself to set the defaults non-persistently. If you look for "[NSUserDefaults standardUserDefaults] setVolatileDomain:..." in DumpRenderTree, I think you can find an example where the binary was changed to do this.
Sam Weinig
Comment 17
2020-11-06 12:24:12 PST
(In reply to Jonathan Bedard from
comment #13
)
> (In reply to Jonathan Bedard from
comment #12
) > > Created
attachment 413453
[details]
> > Patch > > New patch uses `defaults write` in run-api-tests, which I think is cleaner > than trying to strip arguments out of argv before passing them to gtest. > We're obviously enabling GPU Process for any API tests running in parallel > with us, but API tests already break if you run them that way.
Do you have to strip them out? Does gtest break if it sees arguments it doesn't understand? I seem to remember that it would actually return the arguments it doesn't understand, though that could be old knowledge.
Jonathan Bedard
Comment 18
2020-11-06 12:45:30 PST
(In reply to Sam Weinig from
comment #17
)
> (In reply to Jonathan Bedard from
comment #13
) > > (In reply to Jonathan Bedard from
comment #12
) > > > Created
attachment 413453
[details]
> > > Patch > > > > New patch uses `defaults write` in run-api-tests, which I think is cleaner > > than trying to strip arguments out of argv before passing them to gtest. > > We're obviously enabling GPU Process for any API tests running in parallel > > with us, but API tests already break if you run them that way. > > Do you have to strip them out? Does gtest break if it sees arguments it > doesn't understand? I seem to remember that it would actually return the > arguments it doesn't understand, though that could be old knowledge.
I assumed I did, but maybe that assumption is wrong. If we wanted to do this inside the test binary, we would also need to reset the values before exiting, as far as I can tell, which isn't an easy thing to do consistently since we could crash when running a test. And TestWebKitAPI would still be respecting `defaults write` anyways, so I'm not sure what benefit we would be getting to changing those setting inside the binary.
Sam Weinig
Comment 19
2020-11-06 12:48:08 PST
(In reply to Jonathan Bedard from
comment #18
)
> (In reply to Sam Weinig from
comment #17
) > > (In reply to Jonathan Bedard from
comment #13
) > > > (In reply to Jonathan Bedard from
comment #12
) > > > > Created
attachment 413453
[details]
> > > > Patch > > > > > > New patch uses `defaults write` in run-api-tests, which I think is cleaner > > > than trying to strip arguments out of argv before passing them to gtest. > > > We're obviously enabling GPU Process for any API tests running in parallel > > > with us, but API tests already break if you run them that way. > > > > Do you have to strip them out? Does gtest break if it sees arguments it > > doesn't understand? I seem to remember that it would actually return the > > arguments it doesn't understand, though that could be old knowledge. > > I assumed I did, but maybe that assumption is wrong. > > If we wanted to do this inside the test binary, we would also need to reset > the values before exiting, as far as I can tell, which isn't an easy thing > to do consistently since we could crash when running a test. And > TestWebKitAPI would still be respecting `defaults write` anyways, so I'm not > sure what benefit we would be getting to changing those setting inside the > binary.
No need to reset them when exiting if you use the volatile domain. The whole point of it is that they don't persist.
Jonathan Bedard
Comment 20
2020-11-06 12:54:54 PST
(In reply to Darin Adler from
comment #14
)
> Does this mean that if you hit Control-C part-way through, you end up in GPU > Process mode? If so, is that limited to the API tests binary, without > affecting Safari, MiniBrowser, WebKitTestRunner, or DumpRenderTree?
It's possible to find yourself in this state (although, it would only be effecting TestWebKitAPI and TestWTF, no other applications, since it's keyed to them specifically), but a Control-C wouldn't be sufficient to put you in this state because we're re-setting those values in the finally block, which runs as exceptions are raised. You would need to kill the script to find yourself in that state.
Jonathan Bedard
Comment 21
2020-11-06 13:06:56 PST
Created
attachment 413471
[details]
Patch
Alexey Proskuryakov
Comment 22
2020-11-06 13:17:27 PST
TestWebKitAPI uses the volatile domain too, see mainMac.mm.
Jonathan Bedard
Comment 23
2020-11-06 14:32:47 PST
(In reply to Alexey Proskuryakov from
comment #22
)
> TestWebKitAPI uses the volatile domain too, see mainMac.mm.
Looking at this now, will have a patch up soon.
Jonathan Bedard
Comment 24
2020-11-06 16:21:25 PST
Created
attachment 413496
[details]
Patch
Jonathan Bedard
Comment 25
2020-11-06 16:22:44 PST
(In reply to Jonathan Bedard from
comment #24
)
> Created
attachment 413496
[details]
> Patch
Turns out gtest does ignore command line options it doesn't recognize. Which makes this pretty easy.
Tim Horton
Comment 26
2020-11-12 11:48:48 PST
Comment on
attachment 413496
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=413496&action=review
> Source/WTF/Scripts/Preferences/WebPreferencesDebug.yaml:35 > +CaptureAudioInGPUProcessEnabled:
I think, unfortunately, that moving them here means the switches in Safari and MiniBrowser are gone?
Jonathan Bedard
Comment 27
2020-11-12 11:53:12 PST
(In reply to Tim Horton from
comment #26
)
> Comment on
attachment 413496
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=413496&action=review
> > > Source/WTF/Scripts/Preferences/WebPreferencesDebug.yaml:35 > > +CaptureAudioInGPUProcessEnabled: > > I think, unfortunately, that moving them here means the switches in Safari > and MiniBrowser are gone?
Yes. Not really sure there is another way to go, though, unless we make all debug preferences show up as switches there (which seems reasonable to me, but I'm no expert here)
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug