Bug 218296 – [GPUProcess] Override GPUProcess defaults for API testing

• Component: Tools / Tests
• Status: NEW

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/70771368>

[Jonathan Bedard]

Created attachment 412548 [details]
Patch

[Sam Weinig]

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]

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]

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]

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]

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

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

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]

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

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

Created attachment 413453 [details]
Patch

[Jonathan Bedard]

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

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

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

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

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

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

Created attachment 413471 [details]
Patch

[Alexey Proskuryakov]

TestWebKitAPI uses the volatile domain too, see mainMac.mm.

[Jonathan Bedard]

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

Created attachment 413496 [details]
Patch

[Jonathan Bedard]

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

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