Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1914233006:
Implement offscreenCanvas.getContext('webgl') on a worker thread (Closed)
|
Created:
4 years, 7 months ago by xidachen Modified:
4 years, 7 months ago Reviewers:
Ken Russell (switch to Gerrit), bajones, sof, Justin Novosad, danakj, Zhenyao Mo, haraken, pfeldman CC:
chromium-reviews, blink-reviews, haraken Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
In this CL, we also Make BindToCurrentThread() optional in
createOffscreenGraphicsContext3DProvider.
Obtain a contextProvider on a worker thread involves these steps:
1. postTask to the main thread.
2. main thread calls createOffscreenGraphicsContext3DProvider, which
implicitly calls BindToCurrentThread().
3. main thread signal that contextProvider is ready.
4. worker thread calls contextProvider->DetachFromThread(), and then
contextProvider->BindToCurrentThread().
We can see that BindToCurrentThread() gets called twice. If we make
the BindToCurrentThread() call optional in createOffscreenGraphicsContext3DProvider,
then the worker thread will be the first one calling BindToCurrentThread(),
and there will be no need to call DetachFromThread().
Besides that, we made the the PersistentHeapHashSet and PersistentHeapHashMap
in the WebGLRenderingContextBase to be per-thread. The reason is
that we do not want the contexts created on the main thread and on
worker threads to share the same hashset or hashmap.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/2ec178e3f4f22a0806b5a13aff43a8d570e6c44c
Cr-Commit-Position: refs/heads/master@{#392425}
Patch Set 1 #
Total comments: 5
Patch Set 2 : needs to see diff #Patch Set 3 : bots, try #Patch Set 4 : should compile #Patch Set 5 : should work #
Total comments: 2
Patch Set 6 : per thread hashset&hashmap #Patch Set 7 : totally works #
Total comments: 6
Patch Set 8 : runTerminationGC() calls releaseStaticPersistentNodes() #
Total comments: 8
Patch Set 9 : address kbr@'s comments, and limit maxGLActiveContexts=4 per worker thread #
Total comments: 11
Patch Set 10 : address pfeldman's comments #Patch Set 11 : address sof@'s comments #Patch Set 12 : rebase #
Total comments: 4
Patch Set 13 : temporary change the layout test so it doesn't time out, DONOT COMMIT THIS PS #Patch Set 14 : remove changes in threadstate.cpp for debugging #Patch Set 15 : expose WebGLRenderingContext to worker #Patch Set 16 : make expose to worker experimental #
Total comments: 5
Patch Set 17 : address comments and update global-interface results #
Total comments: 4
Patch Set 18 : address kbr@'s comment #Messages
Total messages: 82 (23 generated)
Description was changed from
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
BUG=602391
==========
to
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
xidachen@chromium.org changed reviewers: + bajones@chromium.org, junov@chromium.org, kbr@chromium.org, zmo@chromium.org
PTAL https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515: gpu::gles2::GLES2Interface* gl = contextProvider->contextGL(); The program works fine on a Release build, but crashes on a debug build. This is the crash point, the stack trace eventually gets to: FATAL:context_provider_command_buffer.cc(120)] Check failed: context_thread_checker_.CalledOnValidThread() zm@, bajones@: could you take a look? Thank you.
https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515: gpu::gles2::GLES2Interface* gl = contextProvider->contextGL(); On 2016/04/29 15:58:21, xidachen wrote: > The program works fine on a Release build, but crashes on a debug build. This is > the crash point, the stack trace eventually gets to: > > FATAL:context_provider_command_buffer.cc(120)] Check failed: > context_thread_checker_.CalledOnValidThread() > > zm@, bajones@: could you take a look? Thank you. Many of our bots run with DCHECKs enabled, so if this is still crashing in debug it's not appropriate to commit. Is it still failing even though you're doing the post task to the main thread?
https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515: gpu::gles2::GLES2Interface* gl = contextProvider->contextGL(); Yes, the current code crashes. I am quite surprised, as you can see, contextProvider is created on the main thread, and this function is also called on main thread. In theory, it should not hit the DCHECK point.
https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515: gpu::gles2::GLES2Interface* gl = contextProvider->contextGL(); On 2016/04/29 15:58:21, xidachen wrote: > The program works fine on a Release build, but crashes on a debug build. This is > the crash point, the stack trace eventually gets to: > > FATAL:context_provider_command_buffer.cc(120)] Check failed: > context_thread_checker_.CalledOnValidThread() > > zm@, bajones@: could you take a look? Thank you. You just have to call ContextProviderCommandBuffer::DetachFromThread() to fix that, but this is not the right place for it. There is an important design flaw here: The gl interface should always be used from the same thread. So the calls to the gl interface below need to happen on the thread that is going to be using the gl interface, which is not always the main thread. This function is on the main thread.
https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515: gpu::gles2::GLES2Interface* gl = contextProvider->contextGL(); Here is what I propose: We change the WebGraphicsContext3DProvider::contextGL() to WebGraphicsContext3DProvider::contextGL(bool shouldDetachFromThread=false), then we change its implementation such that it would call DetachFromThread() only if the input parameter is true. WDYT?
On 2016/04/30 01:02:33, xidachen wrote: > https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/1914233006/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:515: > gpu::gles2::GLES2Interface* gl = contextProvider->contextGL(); > > Here is what I propose: > > We change the WebGraphicsContext3DProvider::contextGL() to > WebGraphicsContext3DProvider::contextGL(bool shouldDetachFromThread=false), then > we change its implementation such that it would call DetachFromThread() only if > the input parameter is true. WDYT? I don't think that works. The name "DetachFromThread" is missleading. What it really does is DetachFromOldThreadAndAttachToCurrentThread. You need to call it once you have passed the context to the thread where it is going to be used.
Xida: I suggest you specify the GYP_DEFINE or GN arg "dcheck_always_on=1" in your Release builds. This will help you catch more bugs.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
to
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
xidachen@chromium.org changed reviewers: + danakj@chromium.org
PTAL, PS#5 should work.
Nit + lgtm for platform/graphics and LayoutTests https://codereview.chromium.org/1914233006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:503: void WebGLRenderingContextBase::createWebGraphicsContext3DProviderMainThread(WebGLRenderingContextBase::createWebGraphicsContext3DProviderUtils* createUtils, WaitableEvent* waitableEvent) "OnMainThread"
https://codereview.chromium.org/1914233006/diff/80001/content/renderer/webgra... File content/renderer/webgraphicscontext3d_provider_impl.cc (right): https://codereview.chromium.org/1914233006/diff/80001/content/renderer/webgra... content/renderer/webgraphicscontext3d_provider_impl.cc:24: void WebGraphicsContext3DProviderImpl::DetachFromThread() { I'm very uncomfortable with using these primitives. Please refactor RendererBlinkPlatformImpl::createOffscreenGraphicsContext3DProvider so that, optionally, BindToCurrentThread isn't called in the first place. Then the worker thread can be the first to call BindToCurrentThread on the newly-created ContextProviderCommandBuffer.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/120001
Description was changed from
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
to
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
In this CL, we also Make BindToCurrentThread() optional in
createOffscreenGraphicsContext3DProvider.
Obtain a contextProvider on a worker thread involves these steps:
1. postTask to the main thread.
2. main thread calls createOffscreenGraphicsContext3DProvider, which
implicitly calls BindToCurrentThread().
3. main thread signal that contextProvider is ready.
4. worker thread calls contextProvider->DetachFromThread(), and then
contextProvider->BindToCurrentThread().
We can see that BindToCurrentThread() gets called twice. If we make
the BindToCurrentThread() call optional in
createOffscreenGraphicsContext3DProvider,
then the worker thread will be the first one calling BindToCurrentThread(),
and there will be no need to call DetachFromThread().
Besides that, we made the the PersistentHeapHashSet and PersistentHeapHashMap
in the WebGLRenderingContextBase to be per-thread. The reason is
that we do not want the contexts created on the main thread and on
worker threads to share the same hashset or hashmap.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
xidachen@chromium.org changed reviewers: + pfeldman@chromium.org, sigbjornf@opera.com
kbr@: I have run all layout tests under fast/canvas/ on debug build and everything is working fine. If the dry run passes, then I think it should be good. https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:226: sof@: is it okay to call isTerminating() here? The reason I put it here is that when the worker thread shuts down, it goes to here, and at line 233 where it tries to see activeContexts().size(), the call activeContexts() creates a new PersistentHeapHashSet, and the runTerminationGC() fails.
https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:226: On 2016/05/06 15:15:28, xidachen wrote: > sof@: is it okay to call isTerminating() here? The reason I put it here is that > when the worker thread shuts down, it goes to here, and at line 233 where it > tries to see activeContexts().size(), the call activeContexts() creates a new > PersistentHeapHashSet, and the runTerminationGC() fails. Because the !currentCount assert fails, or something else?
https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:226: On 2016/05/06 15:36:44, sof wrote: > On 2016/05/06 15:15:28, xidachen wrote: > > sof@: is it okay to call isTerminating() here? The reason I put it here is > that > > when the worker thread shuts down, it goes to here, and at line 233 where it > > tries to see activeContexts().size(), the call activeContexts() creates a new > > PersistentHeapHashSet, and the runTerminationGC() fails. > > Because the !currentCount assert fails, or something else? Yes, it is hitting the !currentCount, and I looked at the type of PersistentNode, it is the HashSet and HashMap in here.
https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:226: On 2016/05/06 15:38:05, xidachen wrote: > On 2016/05/06 15:36:44, sof wrote: > > On 2016/05/06 15:15:28, xidachen wrote: > > > sof@: is it okay to call isTerminating() here? The reason I put it here is > > that > > > when the worker thread shuts down, it goes to here, and at line 233 where it > > > tries to see activeContexts().size(), the call activeContexts() creates a > new > > > PersistentHeapHashSet, and the runTerminationGC() fails. > > > > Because the !currentCount assert fails, or something else? > > Yes, it is hitting the !currentCount, and I looked at the type of > PersistentNode, it is the HashSet and HashMap in here. thanks, I suggest we instead try to call releaseStaticPersistentNodes() (also) inside the loop that runTerminationGC() has. I think it will work (but untested.) That way we avoid having to expose ThreadState internal states like isTerminating() outside the Oilpan infrastructure.
https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:226: On 2016/05/06 15:45:15, sof wrote: > On 2016/05/06 15:38:05, xidachen wrote: > > On 2016/05/06 15:36:44, sof wrote: > > > On 2016/05/06 15:15:28, xidachen wrote: > > > > sof@: is it okay to call isTerminating() here? The reason I put it here is > > > that > > > > when the worker thread shuts down, it goes to here, and at line 233 where > it > > > > tries to see activeContexts().size(), the call activeContexts() creates a > > new > > > > PersistentHeapHashSet, and the runTerminationGC() fails. > > > > > > Because the !currentCount assert fails, or something else? > > > > Yes, it is hitting the !currentCount, and I looked at the type of > > PersistentNode, it is the HashSet and HashMap in here. > > thanks, I suggest we instead try to call releaseStaticPersistentNodes() (also) > inside the loop that runTerminationGC() has. I think it will work (but > untested.) > > That way we avoid having to expose ThreadState internal states like > isTerminating() outside the Oilpan infrastructure. That works! Thank you.
https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:226: On 2016/05/06 15:59:35, xidachen wrote: > On 2016/05/06 15:45:15, sof wrote: > > On 2016/05/06 15:38:05, xidachen wrote: > > > On 2016/05/06 15:36:44, sof wrote: > > > > On 2016/05/06 15:15:28, xidachen wrote: > > > > > sof@: is it okay to call isTerminating() here? The reason I put it here > is > > > > that > > > > > when the worker thread shuts down, it goes to here, and at line 233 > where > > it > > > > > tries to see activeContexts().size(), the call activeContexts() creates > a > > > new > > > > > PersistentHeapHashSet, and the runTerminationGC() fails. > > > > > > > > Because the !currentCount assert fails, or something else? > > > > > > Yes, it is hitting the !currentCount, and I looked at the type of > > > PersistentNode, it is the HashSet and HashMap in here. > > > > thanks, I suggest we instead try to call releaseStaticPersistentNodes() (also) > > inside the loop that runTerminationGC() has. I think it will work (but > > untested.) > > > > That way we avoid having to expose ThreadState internal states like > > isTerminating() outside the Oilpan infrastructure. > > That works! Thank you. It would be preferable to land that ThreadState change separately - a case we hadn't come across before. https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if (activeContexts().contains(this)) This shouldn't be needed -- if this context object is allowed to destruct, that means there can be no reference to it remaining. Including from these two global maps which keeps weak references.
https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if (activeContexts().contains(this)) On 2016/05/06 16:06:17, sof wrote: > This shouldn't be needed -- if this context object is allowed to destruct, that > means there can be no reference to it remaining. Including from these two global > maps which keeps weak references. I was actually going to ask for your suggestions. If I comment our this section, I am hitting this assertion on willDestroyContext: ASSERT(!activeContexts().contains(context)); This is specific to a worker thread.
Excellent work Xida. LGTM modulo a naming convention issue. Please tell me if I can help debugging the remaining correctness issues related to Oilpan. https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if (activeContexts().contains(this)) On 2016/05/06 16:54:43, xidachen wrote: > On 2016/05/06 16:06:17, sof wrote: > > This shouldn't be needed -- if this context object is allowed to destruct, > that > > means there can be no reference to it remaining. Including from these two > global > > maps which keeps weak references. > > I was actually going to ask for your suggestions. If I comment our this section, > I am hitting this assertion on willDestroyContext: > ASSERT(!activeContexts().contains(context)); > > This is specific to a worker thread. Interesting. What's the order of processing when clearing weak refs in Oilpan? Is the target object destroyed and then all weak refs to it cleared? It sounds like this is happening, as opposed to vice versa. https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1096: static PassOwnPtr<WebGraphicsContext3DProvider> createWebGraphicsContext3DProviderInternal(HTMLCanvasElement*, ScriptState*, WebGLContextAttributes, unsigned); WebGraphicsContext3D has been removed as a concept from the codebase thanks to danakj@ and the last remaining references like WebGraphicsContext3DProvider are due to be renamed. Let's please not add more uses of the name. Everywhere WebGraphicsContext3D is referenced, let's just use Context. Let's call this method createContextProviderInternal. https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1097: class createWebGraphicsContext3DProviderUtils { Classes' names should be capitalized. This should be something like ContextProviderCreationInfo or ContextProviderCreationUtils.
https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if (activeContexts().contains(this)) On 2016/05/06 16:54:43, xidachen wrote: > On 2016/05/06 16:06:17, sof wrote: > > This shouldn't be needed -- if this context object is allowed to destruct, > that > > means there can be no reference to it remaining. Including from these two > global > > maps which keeps weak references. > > I was actually going to ask for your suggestions. If I comment our this section, > I am hitting this assertion on willDestroyContext: > ASSERT(!activeContexts().contains(context)); > > This is specific to a worker thread. The only reason I can think of why that might happen is if thread-local weak processing doesn't happen when terminating a worker thread, for some reason. But looking at the code, it ought to, so I don't understand what is going on here. But it's a bug, all weak processing must have happened before finalizers run.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/160001
pfeldman@: could you take a look at contents/ and components/? Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridgeTest.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridgeTest.cpp:90: bool BindToCurrentThread() override { return false; } start with lower case. https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:244: bool BindToCurrentThread() override { return false; } start with lower case. https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:244: releaseStaticPersistentNodes(); What is this about? Could you send it to the heap folks for review? https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebGraphicsContext3DProvider.h:53: virtual bool BindToCurrentThread() = 0; start method names with lowercase.
https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if (activeContexts().contains(this)) On 2016/05/06 18:43:34, sof wrote: > On 2016/05/06 16:54:43, xidachen wrote: > > On 2016/05/06 16:06:17, sof wrote: > > > This shouldn't be needed -- if this context object is allowed to destruct, > > that > > > means there can be no reference to it remaining. Including from these two > > global > > > maps which keeps weak references. > > > > I was actually going to ask for your suggestions. If I comment our this > section, > > I am hitting this assertion on willDestroyContext: > > ASSERT(!activeContexts().contains(context)); > > > > This is specific to a worker thread. > > The only reason I can think of why that might happen is if thread-local weak > processing doesn't happen when terminating a worker thread, for some reason. But > looking at the code, it ought to, so I don't understand what is going on here. > > But it's a bug, all weak processing must have happened before finalizers run. I now understand what's missing here, https://codereview.chromium.org/1957523007/ addresses. Will tidy up and send it along tomorrow.
https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:244: releaseStaticPersistentNodes(); On 2016/05/06 21:20:40, pfeldman wrote: > What is this about? Could you send it to the heap folks for review? sof@: Thank you for your CL that clears static persistents. With your new CL, I guess we don't need this anymore?
https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:244: releaseStaticPersistentNodes(); On 2016/05/06 21:41:54, xidachen wrote: > On 2016/05/06 21:20:40, pfeldman wrote: > > What is this about? Could you send it to the heap folks for review? > > sof@: Thank you for your CL that clears static persistents. > With your new CL, I guess we don't need this anymore? It is still needed, a destructor might still create a thread-local singleton while shutting down, so we need to be more insistent about releasing those fresh singletons while cleanly shutting down the worker. But as #32 suggested, this particular change belongs in a separate CL -- I can take care of that. The other issue is that the release itself wasn't comprehensive enough for these persistent collection objects, so we'll need to fix that as well. (We do not have any other ThreadSpecific<>s over persistent collections in the codebase.)
https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html:12: if (toString.call(ctx1) != '[object OffscreenCanvasRenderingContext2D]') { Expressing these type checks as if (!(ctx1 instanceof OffscreenCanvasRenderingContext2D)) would arguably be more idiomatic. https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:196: unsigned maxGLContexts; Could you create a local method for this next to where maxGLActiveContextsOnWorker is defined & declared? https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:559: WaitableEvent waitableEvent; Could you abstract this out as createContextProviderFromWorkerThread()? All the scaffolding code around posting this task obscures reading. Is it acceptable for this creation operation to be synchronous, or does that aspect merit a TODO? https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h:1097: class ContextProviderCreationInfo { Can we avoid inlining this local class in an already large header file?
pfeldman@, sof@: all your comments are addressed. haraken@: I took sof@'s suggestion and create a util class that is used in WebGLRenderingContextBase, so gypi change is necessary under modules/. https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:559: WaitableEvent waitableEvent; On 2016/05/07 07:21:42, sof wrote: > Could you abstract this out as createContextProviderFromWorkerThread()? All the > scaffolding code around posting this task obscures reading. > > Is it acceptable for this creation operation to be synchronous, or does that > aspect merit a TODO? Yes, it is definitely a future work to make this asynchronous. I put a TODO comment here. https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: releaseStaticPersistentNodes(); sof@, haraken@: without this line, the layout test hits the ASSERT(!currentCount);
xidachen@chromium.org changed reviewers: + haraken@chromium.org
+haraken@: gypi changes under modules/
https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if (activeContexts().contains(this)) On 2016/05/06 21:37:23, sof wrote: > On 2016/05/06 18:43:34, sof wrote: > > On 2016/05/06 16:54:43, xidachen wrote: > > > On 2016/05/06 16:06:17, sof wrote: > > > > This shouldn't be needed -- if this context object is allowed to destruct, > > > that > > > > means there can be no reference to it remaining. Including from these two > > > global > > > > maps which keeps weak references. > > > > > > I was actually going to ask for your suggestions. If I comment our this > > section, > > > I am hitting this assertion on willDestroyContext: > > > ASSERT(!activeContexts().contains(context)); > > > > > > This is specific to a worker thread. > > > > The only reason I can think of why that might happen is if thread-local weak > > processing doesn't happen when terminating a worker thread, for some reason. > But > > looking at the code, it ought to, so I don't understand what is going on here. > > > > > But it's a bug, all weak processing must have happened before finalizers run. > > I now understand what's missing here, > https://codereview.chromium.org/1957523007/ addresses. Will tidy up and send it > along tomorrow. If the CL is rebased >= r392263, the fix will be in scope & this shouldn't be needed.
https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: releaseStaticPersistentNodes(); On 2016/05/07 17:07:49, xidachen wrote: > sof@, haraken@: without this line, the layout test hits the > ASSERT(!currentCount); Really, isn't the releaseStaticPersistentNodes() call below sufficient?
On 2016/05/07 17:09:50, sof wrote: > https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp > (right): > > https://codereview.chromium.org/1914233006/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:1109: if > (activeContexts().contains(this)) > On 2016/05/06 21:37:23, sof wrote: > > On 2016/05/06 18:43:34, sof wrote: > > > On 2016/05/06 16:54:43, xidachen wrote: > > > > On 2016/05/06 16:06:17, sof wrote: > > > > > This shouldn't be needed -- if this context object is allowed to > destruct, > > > > that > > > > > means there can be no reference to it remaining. Including from these > two > > > > global > > > > > maps which keeps weak references. > > > > > > > > I was actually going to ask for your suggestions. If I comment our this > > > section, > > > > I am hitting this assertion on willDestroyContext: > > > > ASSERT(!activeContexts().contains(context)); > > > > > > > > This is specific to a worker thread. > > > > > > The only reason I can think of why that might happen is if thread-local weak > > > processing doesn't happen when terminating a worker thread, for some reason. > > But > > > looking at the code, it ought to, so I don't understand what is going on > here. > > > > > > > > But it's a bug, all weak processing must have happened before finalizers > run. > > > > I now understand what's missing here, > > https://codereview.chromium.org/1957523007/ addresses. Will tidy up and send > it > > along tomorrow. > > If the CL is rebased >= r392263, the fix will be in scope & this shouldn't be > needed. Yes, I did rebase it because I saw you commit your new patch moments ago. As you can see, right now in ThreadState.cpp, the while loop already has a releaseStaticPersistentNodes(); line.
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/220001
https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: releaseStaticPersistentNodes(); On 2016/05/07 17:11:23, sof wrote: > On 2016/05/07 17:07:49, xidachen wrote: > > sof@, haraken@: without this line, the layout test hits the > > ASSERT(!currentCount); > > Really, isn't the releaseStaticPersistentNodes() call below sufficient? I just tried something, without this line here, and change the while loop to be while(currentCount != oldCount || currentCount) in that case, we will for sure not hit that ASSERT. WDYT?
https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: releaseStaticPersistentNodes(); On 2016/05/07 17:28:30, xidachen wrote: > On 2016/05/07 17:11:23, sof wrote: > > On 2016/05/07 17:07:49, xidachen wrote: > > > sof@, haraken@: without this line, the layout test hits the > > > ASSERT(!currentCount); > > > > Really, isn't the releaseStaticPersistentNodes() call below sufficient? > > I just tried something, without this line here, and change the while loop to be > > while(currentCount != oldCount || currentCount) > > in that case, we will for sure not hit that ASSERT. > > WDYT? Not a good idea, as you will be perpetually stuck in the loop if there are persistents alive that aren't released. The loop will be executed at least once, so the claim that a test is failing doesn't make sense to me.
On 2016/05/07 17:33:32, sof wrote: > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: > releaseStaticPersistentNodes(); > On 2016/05/07 17:28:30, xidachen wrote: > > On 2016/05/07 17:11:23, sof wrote: > > > On 2016/05/07 17:07:49, xidachen wrote: > > > > sof@, haraken@: without this line, the layout test hits the > > > > ASSERT(!currentCount); > > > > > > Really, isn't the releaseStaticPersistentNodes() call below sufficient? > > > > I just tried something, without this line here, and change the while loop to > be > > > > while(currentCount != oldCount || currentCount) > > > > in that case, we will for sure not hit that ASSERT. > > > > WDYT? > > Not a good idea, as you will be perpetually stuck in the loop if there are > persistents alive that aren't released. > > The loop will be executed at least once, so the claim that a test is failing > doesn't make sense to me. I see, I agree that it could stuck in there. I temporary change the layout test to use toString thing because without that I am having a timeout. I will investigate on that further. We can take a look at the result of PS#13.
On 2016/05/07 17:36:05, xidachen wrote: > On 2016/05/07 17:33:32, sof wrote: > > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: > > releaseStaticPersistentNodes(); > > On 2016/05/07 17:28:30, xidachen wrote: > > > On 2016/05/07 17:11:23, sof wrote: > > > > On 2016/05/07 17:07:49, xidachen wrote: > > > > > sof@, haraken@: without this line, the layout test hits the > > > > > ASSERT(!currentCount); > > > > > > > > Really, isn't the releaseStaticPersistentNodes() call below sufficient? > > > > > > I just tried something, without this line here, and change the while loop to > > be > > > > > > while(currentCount != oldCount || currentCount) > > > > > > in that case, we will for sure not hit that ASSERT. > > > > > > WDYT? > > > > Not a good idea, as you will be perpetually stuck in the loop if there are > > persistents alive that aren't released. > > > > The loop will be executed at least once, so the claim that a test is failing > > doesn't make sense to me. > > I see, I agree that it could stuck in there. > > I temporary change the layout test to use toString thing because without that I > am having a timeout. I will investigate on that further. We can take a look at > the result of PS#13. WebGLRenderingContext isn't exposed in a worker context in this CL afaict, so you may have to add [Exposed=(Window, Worker)] to its .idl
On 2016/05/07 17:42:48, sof wrote: > On 2016/05/07 17:36:05, xidachen wrote: > > On 2016/05/07 17:33:32, sof wrote: > > > > > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: > > > releaseStaticPersistentNodes(); > > > On 2016/05/07 17:28:30, xidachen wrote: > > > > On 2016/05/07 17:11:23, sof wrote: > > > > > On 2016/05/07 17:07:49, xidachen wrote: > > > > > > sof@, haraken@: without this line, the layout test hits the > > > > > > ASSERT(!currentCount); > > > > > > > > > > Really, isn't the releaseStaticPersistentNodes() call below sufficient? > > > > > > > > I just tried something, without this line here, and change the while loop > to > > > be > > > > > > > > while(currentCount != oldCount || currentCount) > > > > > > > > in that case, we will for sure not hit that ASSERT. > > > > > > > > WDYT? > > > > > > Not a good idea, as you will be perpetually stuck in the loop if there are > > > persistents alive that aren't released. > > > > > > The loop will be executed at least once, so the claim that a test is failing > > > doesn't make sense to me. > > > > I see, I agree that it could stuck in there. > > > > I temporary change the layout test to use toString thing because without that > I > > am having a timeout. I will investigate on that further. We can take a look at > > the result of PS#13. > > WebGLRenderingContext isn't exposed in a worker context in this CL afaict, so > you may have to add [Exposed=(Window, Worker)] to its .idl This page: https://build.chromium.org/p/tryserver.blink/builders/mac_blink_dbg/builds/45... is the output for "mac_blink_dbg" for PS14, I used !currentCount to search and it is there.
On 2016/05/07 18:32:42, xidachen wrote: > On 2016/05/07 17:42:48, sof wrote: > > On 2016/05/07 17:36:05, xidachen wrote: > > > On 2016/05/07 17:33:32, sof wrote: > > > > > > > > > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/platform/heap/ThreadState.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1914233006/diff/220001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/platform/heap/ThreadState.cpp:229: > > > > releaseStaticPersistentNodes(); > > > > On 2016/05/07 17:28:30, xidachen wrote: > > > > > On 2016/05/07 17:11:23, sof wrote: > > > > > > On 2016/05/07 17:07:49, xidachen wrote: > > > > > > > sof@, haraken@: without this line, the layout test hits the > > > > > > > ASSERT(!currentCount); > > > > > > > > > > > > Really, isn't the releaseStaticPersistentNodes() call below > sufficient? > > > > > > > > > > I just tried something, without this line here, and change the while > loop > > to > > > > be > > > > > > > > > > while(currentCount != oldCount || currentCount) > > > > > > > > > > in that case, we will for sure not hit that ASSERT. > > > > > > > > > > WDYT? > > > > > > > > Not a good idea, as you will be perpetually stuck in the loop if there are > > > > persistents alive that aren't released. > > > > > > > > The loop will be executed at least once, so the claim that a test is > failing > > > > doesn't make sense to me. > > > > > > I see, I agree that it could stuck in there. > > > > > > I temporary change the layout test to use toString thing because without > that > > I > > > am having a timeout. I will investigate on that further. We can take a look > at > > > the result of PS#13. > > > > WebGLRenderingContext isn't exposed in a worker context in this CL afaict, so > > you may have to add [Exposed=(Window, Worker)] to its .idl > > This page: > https://build.chromium.org/p/tryserver.blink/builders/mac_blink_dbg/builds/45... > > is the output for "mac_blink_dbg" for PS14, I used !currentCount to search and > it is there. thanks, found time to access to my dev box and have a closer look. https://codereview.chromium.org/1963453002/ addresses and avoids reaching these false fixed points.
content, components, WebKit/public lgtm
https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html (left): https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html:14: self.postMessage("aCanvas.getContext('2d') does not return [object OffscreenCanvasRenderingContext2D]"); Can you update the test failure descriptions to match? https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h (right): https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h:15: class MODULES_EXPORT WebGLContextProviderCreationInfo { This class can be made local to WebGLRenderingContextBase instead, and defined next-door to where it is used in WebGLRenderingContextBase.cpp
https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h (right): https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h:15: class MODULES_EXPORT WebGLContextProviderCreationInfo { On 2016/05/08 07:05:00, sof wrote: > This class can be made local to WebGLRenderingContextBase instead, and defined > next-door to where it is used in WebGLRenderingContextBase.cpp Sorry, I must have mis-interpret your previous comments of avoid inlining this class. Do you mean declare this class in the WebGLRenderingContextBase.h, and define it in the source file?
https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h (right): https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h:15: class MODULES_EXPORT WebGLContextProviderCreationInfo { On 2016/05/08 17:17:45, xidachen wrote: > On 2016/05/08 07:05:00, sof wrote: > > This class can be made local to WebGLRenderingContextBase instead, and defined > > next-door to where it is used in WebGLRenderingContextBase.cpp > > Sorry, I must have mis-interpret your previous comments of avoid inlining this > class. Do you mean declare this class in the WebGLRenderingContextBase.h, and > define it in the source file? WebGLRenderingContextBase.h: class WebGLRenderingContextBase : ... { .... private: ... class ContextProviderCreationInfo; ... }; WebGLRenderingContextBase.cpp: namespace blink { .... class WebGLRenderingContextBase::ContextProviderCreationInfo { ... like here.. }; ... }
https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h (right): https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h:15: class MODULES_EXPORT WebGLContextProviderCreationInfo { On 2016/05/08 17:30:21, sof wrote: > On 2016/05/08 17:17:45, xidachen wrote: > > On 2016/05/08 07:05:00, sof wrote: > > > This class can be made local to WebGLRenderingContextBase instead, and > defined > > > next-door to where it is used in WebGLRenderingContextBase.cpp > > > > Sorry, I must have mis-interpret your previous comments of avoid inlining this > > class. Do you mean declare this class in the WebGLRenderingContextBase.h, and > > define it in the source file? > > WebGLRenderingContextBase.h: > > class WebGLRenderingContextBase : ... > { > .... > private: > ... > class ContextProviderCreationInfo; > ... > }; > > WebGLRenderingContextBase.cpp: > > namespace blink { > .... > class WebGLRenderingContextBase::ContextProviderCreationInfo { > ... like here.. > }; > ... > } Thank you. I have made changes accordingly. Also, with your new CL that changes ThreadState.cpp, the layout tests works perfectly fine. I think this CL is ready now.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/05/08 18:04:53, xidachen wrote: > https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h > (right): > > https://codereview.chromium.org/1914233006/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/webgl/WebGLContextProviderCreationInfo.h:15: > class MODULES_EXPORT WebGLContextProviderCreationInfo { > On 2016/05/08 17:30:21, sof wrote: > > On 2016/05/08 17:17:45, xidachen wrote: > > > On 2016/05/08 07:05:00, sof wrote: > > > > This class can be made local to WebGLRenderingContextBase instead, and > > defined > > > > next-door to where it is used in WebGLRenderingContextBase.cpp > > > > > > Sorry, I must have mis-interpret your previous comments of avoid inlining > this > > > class. Do you mean declare this class in the WebGLRenderingContextBase.h, > and > > > define it in the source file? > > > > WebGLRenderingContextBase.h: > > > > class WebGLRenderingContextBase : ... > > { > > .... > > private: > > ... > > class ContextProviderCreationInfo; > > ... > > }; > > > > WebGLRenderingContextBase.cpp: > > > > namespace blink { > > .... > > class WebGLRenderingContextBase::ContextProviderCreationInfo { > > ... like here.. > > }; > > ... > > } > > Thank you. I have made changes accordingly. Also, with your new CL that changes > ThreadState.cpp, the layout tests works perfectly fine. I think this CL is ready > now. Looks fine from here, but the CL needs another review from webgl owners to cover the changes made in the later patchsets.
@kbr, @zmo, @bajones: could you take a look at webgl/ changes? Now WebGLRenderingContext is exposed to worker, but label as experimental features.
WebGL parts still LGTM. Thanks again for pushing this through and thanks sigbjornf@ for your help fixing up the Oilpan side. https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html (right): https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html:12: if (!(ctx1 instanceof OffscreenCanvasRenderingContext2D)) { Note: OffscreenCanvasRenderingContext2D is not part of any of the current WHATWG specs or proposals. Either it needs to be specified, or this test needs to be changed to refer to CanvasRenderingContext2D, and the problematic methods on CanvasRenderingContext2D need to be specified to throw exceptions, or similar. https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:565: // TODO(xidachen): make this context creation asynchronous. Making the context creation asynchronous would imply a change to the public API, which I don't think is desired. Should we just remove this TODO?
https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html (right): https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-getContext-in-worker.html:12: if (!(ctx1 instanceof OffscreenCanvasRenderingContext2D)) { On 2016/05/09 18:00:13, Ken Russell wrote: > Note: OffscreenCanvasRenderingContext2D is not part of any of the current WHATWG > specs or proposals. Either it needs to be specified, or this test needs to be > changed to refer to CanvasRenderingContext2D, and the problematic methods on > CanvasRenderingContext2D need to be specified to throw exceptions, or similar. Agreed. I believe junov@ is currently in the process of making OffscreenCanvasRenderingContext2D as an official spec. I will commit this CL as what is now and come back to this issue later. https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp (right): https://codereview.chromium.org/1914233006/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:565: // TODO(xidachen): make this context creation asynchronous. On 2016/05/09 18:00:13, Ken Russell wrote: > Making the context creation asynchronous would imply a change to the public API, > which I don't think is desired. Should we just remove this TODO? Done.
The CQ bit was checked by xidachen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/340001
The CQ bit was unchecked by xidachen@chromium.org
The CQ bit was checked by xidachen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org, pfeldman@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/1914233006/#ps340001 (title: "address kbr@'s comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1914233006/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1914233006/340001
Message was sent while issue was closed.
Description was changed from
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
In this CL, we also Make BindToCurrentThread() optional in
createOffscreenGraphicsContext3DProvider.
Obtain a contextProvider on a worker thread involves these steps:
1. postTask to the main thread.
2. main thread calls createOffscreenGraphicsContext3DProvider, which
implicitly calls BindToCurrentThread().
3. main thread signal that contextProvider is ready.
4. worker thread calls contextProvider->DetachFromThread(), and then
contextProvider->BindToCurrentThread().
We can see that BindToCurrentThread() gets called twice. If we make
the BindToCurrentThread() call optional in
createOffscreenGraphicsContext3DProvider,
then the worker thread will be the first one calling BindToCurrentThread(),
and there will be no need to call DetachFromThread().
Besides that, we made the the PersistentHeapHashSet and PersistentHeapHashMap
in the WebGLRenderingContextBase to be per-thread. The reason is
that we do not want the contexts created on the main thread and on
worker threads to share the same hashset or hashmap.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
to
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
In this CL, we also Make BindToCurrentThread() optional in
createOffscreenGraphicsContext3DProvider.
Obtain a contextProvider on a worker thread involves these steps:
1. postTask to the main thread.
2. main thread calls createOffscreenGraphicsContext3DProvider, which
implicitly calls BindToCurrentThread().
3. main thread signal that contextProvider is ready.
4. worker thread calls contextProvider->DetachFromThread(), and then
contextProvider->BindToCurrentThread().
We can see that BindToCurrentThread() gets called twice. If we make
the BindToCurrentThread() call optional in
createOffscreenGraphicsContext3DProvider,
then the worker thread will be the first one calling BindToCurrentThread(),
and there will be no need to call DetachFromThread().
Besides that, we made the the PersistentHeapHashSet and PersistentHeapHashMap
in the WebGLRenderingContextBase to be per-thread. The reason is
that we do not want the contexts created on the main thread and on
worker threads to share the same hashset or hashmap.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
In this CL, we also Make BindToCurrentThread() optional in
createOffscreenGraphicsContext3DProvider.
Obtain a contextProvider on a worker thread involves these steps:
1. postTask to the main thread.
2. main thread calls createOffscreenGraphicsContext3DProvider, which
implicitly calls BindToCurrentThread().
3. main thread signal that contextProvider is ready.
4. worker thread calls contextProvider->DetachFromThread(), and then
contextProvider->BindToCurrentThread().
We can see that BindToCurrentThread() gets called twice. If we make
the BindToCurrentThread() call optional in
createOffscreenGraphicsContext3DProvider,
then the worker thread will be the first one calling BindToCurrentThread(),
and there will be no need to call DetachFromThread().
Besides that, we made the the PersistentHeapHashSet and PersistentHeapHashMap
in the WebGLRenderingContextBase to be per-thread. The reason is
that we do not want the contexts created on the main thread and on
worker threads to share the same hashset or hashmap.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========
to
==========
Implement offscreenCanvas.getContext('webgl') on a worker thread
Right now, offscreenCanvas.getContext('webgl') works on the main thread
only because it requires GpuChannelHost. In order to make it work on a
worker thread, we let the worker thread issue a waitable event, and
postTask() to main thread, ask main thread to create the contextProvider,
and signal back to the worker thread.
In this CL, we also Make BindToCurrentThread() optional in
createOffscreenGraphicsContext3DProvider.
Obtain a contextProvider on a worker thread involves these steps:
1. postTask to the main thread.
2. main thread calls createOffscreenGraphicsContext3DProvider, which
implicitly calls BindToCurrentThread().
3. main thread signal that contextProvider is ready.
4. worker thread calls contextProvider->DetachFromThread(), and then
contextProvider->BindToCurrentThread().
We can see that BindToCurrentThread() gets called twice. If we make
the BindToCurrentThread() call optional in
createOffscreenGraphicsContext3DProvider,
then the worker thread will be the first one calling BindToCurrentThread(),
and there will be no need to call DetachFromThread().
Besides that, we made the the PersistentHeapHashSet and PersistentHeapHashMap
in the WebGLRenderingContextBase to be per-thread. The reason is
that we do not want the contexts created on the main thread and on
worker threads to share the same hashset or hashmap.
BUG=602391
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
Committed: https://crrev.com/2ec178e3f4f22a0806b5a13aff43a8d570e6c44c
Cr-Commit-Position: refs/heads/master@{#392425}
==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/2ec178e3f4f22a0806b5a13aff43a8d570e6c44c Cr-Commit-Position: refs/heads/master@{#392425} |
