Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889393003:
Lazy initialization of the rendering thread in OfflineAudioContext (Closed)
|
Created:
3 years, 7 months ago by hongchan Modified:
3 years, 7 months ago Reviewers:
Raymond Toy CC:
Raymond Toy, blink-reviews, chromium-reviews, haraken, hongchan Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionLazy initialization of the rendering thread in OfflineAudioContext
This CL changes the timing of thread initialization in
OfflineAudioDestinationNode after starting the actual rendering. This
prevents out-of-threads situation from happening particularly when user
creates thousands of OfflineAudioContexts upfront.
BUG=723838, 716800
TEST=LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html
Review-Url: https://codereview.chromium.org/2889393003
Cr-Commit-Position: refs/heads/master@{#474494}
Committed: https://chromium.googlesource.com/chromium/src/+/6d697f84c040b2cf41e6d1f90a7b123c496bd836
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change thread creation location and add layout test #
Total comments: 8
Patch Set 3 : Address feedback #
Total comments: 2
Patch Set 4 : Rewrite layout test #
Total comments: 7
Patch Set 5 : Fixing typo in comment #Patch Set 6 : Splitting a test task into two #Patch Set 7 : Adjusting number of contexts for timed-out trybots #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838 ========== to ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838 TEST=(all layout test passes after this change) ==========
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL. https://codereview.chromium.org/2889393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (left): https://codereview.chromium.org/2889393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:239: The existence of thread and being able to schedule a context suspend are orthogonal. Not sure why this code was here in the first place. WDYT?
Can we also add a layout test where we create 1000 (or something) contexts successfully which we couldn't do before. (Choose some number where it fails on at least one of mac, linux, or win) with stable chrome. https://codereview.chromium.org/2889393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (left): https://codereview.chromium.org/2889393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:239: On 2017/05/19 17:35:06, hongchan wrote: > The existence of thread and being able to schedule a context suspend are > orthogonal. Not sure why this code was here in the first place. > > WDYT? Based on the comments, when rendering is done, the thread goes away, and we're rejecting because you can't suspend a context that as already finished rendering. If that's correct, can you check if we have a test for this? If we don't we probably should. https://codereview.chromium.org/2889393003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp (right): https://codereview.chromium.org/2889393003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.cpp:110: render_thread_ = Platform::Current()->CreateThread("offline audio renderer"); Maybe if we fail to create the thread we can reject the promise? Can't see anything in the spec about this, but it makes sense since promise always have a resolve and a reject.
Description was changed from ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838 TEST=(all layout test passes after this change) ========== to ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838, 716800 TEST=(all layout test passes after this change) ==========
Patchset #2 (id:20001) has been deleted
Please update TEST= in description. Will you handle stopping the render thread in a different CL? https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html (right): https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:13: const audit = Audit.createTaskRunner(); Ah, we should do this in the rest of the code. For another day. https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:22: // Create 2500 contexts in total, 1 context per 5ms. Why 2500? And why every 5 ms? Doesn't this mean the test takes 12.5 sec to run? That's longer than the default 6 sec for a layout test. Why not just create all 2500 all at once? https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (left): https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:234: if (!DestinationHandler().OfflineRenderThread()) { What happened before if I try to suspend a context that has already finished rendering? What happens with this CL?
Description was changed from ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838, 716800 TEST=(all layout test passes after this change) ========== to ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838, 716800 TEST=LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html ==========
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html (right): https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:13: const audit = Audit.createTaskRunner(); On 2017/05/23 17:05:01, Raymond Toy wrote: > Ah, we should do this in the rest of the code. For another day. Yeap. I am testing more ideas from ES6 pattern. This is one of them. https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:22: // Create 2500 contexts in total, 1 context per 5ms. On 2017/05/23 17:05:01, Raymond Toy wrote: > Why 2500? And why every 5 ms? Doesn't this mean the test takes 12.5 sec to > run? That's longer than the default 6 sec for a layout test. > > Why not just create all 2500 all at once? I was simply using the repro code from the bug. I can certainly create all at once. That's much easier actually. Is that what really want? https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (left): https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:234: if (!DestinationHandler().OfflineRenderThread()) { On 2017/05/23 17:05:01, Raymond Toy wrote: > What happened before if I try to suspend a context that has already finished > rendering? What happens with this CL? A good catch. My mistake. I forgot to revert this back.
https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html (right): https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:22: // Create 2500 contexts in total, 1 context per 5ms. On 2017/05/23 18:10:38, hongchan wrote: > On 2017/05/23 17:05:01, Raymond Toy wrote: > > Why 2500? And why every 5 ms? Doesn't this mean the test takes 12.5 sec to > > run? That's longer than the default 6 sec for a layout test. > > > > Why not just create all 2500 all at once? > > I was simply using the repro code from the bug. I can certainly create all at > once. That's much easier actually. Is that what really want? 12.5 sec for the test seems really bad. I think it's ok if creating them all at once passes with this CL and fails without.
https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (left): https://codereview.chromium.org/2889393003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:234: if (!DestinationHandler().OfflineRenderThread()) { On 2017/05/23 18:10:38, hongchan wrote: > On 2017/05/23 17:05:01, Raymond Toy wrote: > > What happened before if I try to suspend a context that has already finished > > rendering? What happens with this CL? > > A good catch. My mistake. I forgot to revert this back. In that case, we want to include a test for this with this CL.
https://codereview.chromium.org/2889393003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html (right): https://codereview.chromium.org/2889393003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:42: should(i, 'The number of created contexts without a crash') This is kind of odd because if it crashes, we'll never get here. Would it be clearer to do something like should(() => { // create all the contexts }, `Creating ${maxNumberOfContexts}`) .notThrow() Still has the problem that if it crashes we never actually reach the notThrow part.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2889393003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html (right): https://codereview.chromium.org/2889393003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html:42: should(i, 'The number of created contexts without a crash') On 2017/05/23 18:38:27, Raymond Toy wrote: > This is kind of odd because if it crashes, we'll never get here. Would it be > clearer to do something like > > should(() => { > // create all the contexts > }, `Creating ${maxNumberOfContexts}`) > .notThrow() > > Still has the problem that if it crashes we never actually reach the notThrow > part. Like you pointed out, the out-of-thread immediately crashes anyway. There is not much of we can do about it. https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html (right): https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html:23: const maxNumberOfContexts = 2500; I've tested up to 5000 with this patch. No crash.
https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html (right): https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html:23: const maxNumberOfContexts = 2500; On 2017/05/23 20:30:19, hongchan wrote: > I've tested up to 5000 with this patch. No crash. Comment (3000) doesn't match code (2500) https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html:49: } I think the creation of the contexts should be its own Audit task, and the rendering should be a different task. https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:235: if (ContextState() == AudioContextState::kClosed) { Still need a test for this.
lgtm, but I still think the creation and the rendering should be two Audit tasks. A minor nit.
https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html (right): https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html:23: const maxNumberOfContexts = 2500; On 2017/05/23 20:37:34, Raymond Toy wrote: > On 2017/05/23 20:30:19, hongchan wrote: > > I've tested up to 5000 with this patch. No crash. > > Comment (3000) doesn't match code (2500) I am cranking this up to 5000. https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-smoke-test.html:49: } On 2017/05/23 20:37:34, Raymond Toy wrote: > I think the creation of the contexts should be its own Audit task, and the > rendering should be a different task. Hmm. I think that's overkill. They're quite trivial to separate into different tasks. https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp (right): https://codereview.chromium.org/2889393003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/webaudio/OfflineAudioContext.cpp:235: if (ContextState() == AudioContextState::kClosed) { On 2017/05/23 20:37:34, Raymond Toy wrote: > Still need a test for this. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webaudio/... It's already there. Functionally the task is exactly testing this code. Now we're getting a correct error message from the test: "PASS Scheduling a suspend after the render completion rejected correctly with InvalidStateError: the rendering is already finished." So fortunately I think we just fixed the issue of the original code.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2889393003/#ps160001 (title: "Splitting a test task into two")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hongchan@chromium.org
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2889393003/#ps180001 (title: "Adjusting number of contexts for timed-out trybots")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495655184875750,
"parent_rev": "05e9e692a561e8008de33784bd4c741ad606bcc7", "commit_rev":
"6d697f84c040b2cf41e6d1f90a7b123c496bd836"}
Message was sent while issue was closed.
Description was changed from ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838, 716800 TEST=LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html ========== to ========== Lazy initialization of the rendering thread in OfflineAudioContext This CL changes the timing of thread initialization in OfflineAudioDestinationNode after starting the actual rendering. This prevents out-of-threads situation from happening particularly when user creates thousands of OfflineAudioContexts upfront. BUG=723838, 716800 TEST=LayoutTests/webaudio/OfflineAudioContext/offlineaudiocontext-thread-creation.html Review-Url: https://codereview.chromium.org/2889393003 Cr-Commit-Position: refs/heads/master@{#474494} Committed: https://chromium.googlesource.com/chromium/src/+/6d697f84c040b2cf41e6d1f90a7b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/6d697f84c040b2cf41e6d1f90a7b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
