Bug 62214 – [Chromium] DumpRenderTree should reset ganesh between tests

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Justin Novosad]

When using DumpRenderTree with GPU-accelerated Skia (platform=chromium-gpu), there appears to be crosstalk between tests because of the persistence of state within ganesh.  This was causing layout tests to pass when run as a part of a batch, while they failed when run in isolation (See discussion in http://code.google.com/p/chromium/issues/detail?id=84165 ).  To prevent this from happening, the state stored in GrContext/GrDrawTarget and derived classes should be reset between tests.

[Keyar Hood]

Created attachment 141022 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Jeff Timanus]

Comment on attachment 141022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141022&action=review

Thanks, Keyar.

Things are mostly looking good.  Just some small nits about style and comments.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:30
> +

Was this line added to conform with the style guide?  If not, then don't add it.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:82
> +// This is so that get() and clear() share the same SharedGraphicsContext3DImpl

Period.

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:46
> +    static void clear();

Make sure to mention that this clears the global context instance corresponding to the get routine, not the ImplThread instance.

> Source/WebKit/chromium/public/WebKit.h:74
> +// This allows us to reset the GraphicsContext between each test

Nit:  Period.

[Keyar Hood]

Comment on attachment 141022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141022&action=review

Thanks for the feedback. I will make the fixes.

>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:30
>> +
> 
> Was this line added to conform with the style guide?  If not, then don't add it.

This was added to conform with the style guide. However, I should not have a newline between "config.h" and "SharedGraphicsContext3D.h".

[Stephen White]

Comment on attachment 141022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141022&action=review

> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:83
> +SharedGraphicsContext3DImpl* GetGlobalInstance()

According to the style guide, I think this should be getGlobalInstance():

"Lower-case the first letter, including all letters in an acronym, in a variable or function name."

Bikeshed:  The name getGlobalInstance() is a bit nebulous:  could it be getImpl() instead, so it at least references part of the class name it's returning?

[Keyar Hood]

Created attachment 141256 [details]
Patch

[WebKit Review Bot]

Comment on attachment 141256 [details]
Patch

Attachment 141256 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12677066

New failing tests:
plugins/embed-attributes-style.html

[WebKit Review Bot]

Created attachment 141312 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Keyar Hood]

Created attachment 141439 [details]
Patch

[Keyar Hood]

Created attachment 148861 [details]
Patch

[Keyar Hood]

It appears there can be a great variability in the run times of the layout tests. For example, when I ran them on my computer using time, the run without the tests took about 40 seconds longer than with the change added. Conversely, the tests with the change ran slower than without the change on the try bots. Further, the layout tests ran faster with the option turned on than with it off. It seems that my change does not have a significant effect on the run time.

[Keyar Hood]

Created attachment 149108 [details]
Patch

[Keyar Hood]

It appears that my change having a timing hit was a false alarm. I will keep an eye on the bots after this lands to make sure it is not a problem.

[James Robinson]

Comment on attachment 149108 [details]
Patch

I feel like this is a move in the wrong direction. We reset test state between tests and any settings that we may have clobbered, but it feels wrong to reset arbitrary WebCore state.  In practice we need the shared context to work across many pages - I don't see any reason why DumpRenderTree should be different.  We don't reset the memory cache between tests.

The motivation for this was a bug in skia where it wasn't correctly tracking its own internal state.  While that turned out to be a pain to track down debug, in the end it was a bug in skia and something that should be tested in skia.  It's not a concern of WebKit - there isn't any state that is being managed by WebKit beyond the context itself.

[Keyar Hood]

(In reply to comment #14)
> (From update of attachment 149108 [details])
> I feel like this is a move in the wrong direction. We reset test state between tests and any settings that we may have clobbered, but it feels wrong to reset arbitrary WebCore state.  In practice we need the shared context to work across many pages - I don't see any reason why DumpRenderTree should be different.  We don't reset the memory cache between tests.
> 
> The motivation for this was a bug in skia where it wasn't correctly tracking its own internal state.  While that turned out to be a pain to track down debug, in the end it was a bug in skia and something that should be tested in skia.  It's not a concern of WebKit - there isn't any state that is being managed by WebKit beyond the context itself.

Would adding this as an option be acceptable?

[James Robinson]

You mean as a way for a specific test to request a ganesh reset, or as an option to the test script to reset?

Either way it feels like testing at the wrong level.  If we want to know if ganesh resets its state properly, that should be done by skia tests.  Adding hooks and support to WebKit to test skia just makes the WebKit testing code more complicated without providing any coverage for WebKit code.

[Keyar Hood]

Testing at the Webkit level, even for things not directly related to webkit code, allows us to detect issues caused by upgrading skia or other third-party libraries.

[James Robinson]

That's true for any component that WebKit depends on - network stack, filesystem, audio, video, etc etc etc.  Do you think we should add WebKit hooks for resetting their state as well?

[Keyar Hood]

(In reply to comment #18)
> That's true for any component that WebKit depends on - network stack, filesystem, audio, video, etc etc etc.  Do you think we should add WebKit hooks for resetting their state as well?

Ideally? Yes; each test would run in an isolated environment. 

Of course, there are circumstances where it might be better to not do this, such as for performance concerns. I could also see reasoning where it is not a high priority (or worth the effort) if bugs related to this state persistence have not been seen (or are rarely seen).

[Stephen White]

(In reply to comment #18)
> That's true for any component that WebKit depends on - network stack, filesystem, audio, video, etc etc etc.  Do you think we should add WebKit hooks for resetting their state as well?

Note that this isn't really just Ganesh.  It resets the entire SharedGraphicsContext3D, including the in-process implementation of the command buffer, and those dizzying layers of abstraction over in WebKit/chromium.  So it's testing those too.

(I don't have a strong feeling one way or another about this change, just wanted to point that out).

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=230587