Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498033002:
Media element: avoid v8 allocations in hasPendingActivity(). (Closed)
|
Created:
4 years, 1 month ago by sof Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia element: avoid v8 allocations in hasPendingActivity().
Blink code is not allowed to allocate objects on the v8
heap while its GC calls out to hasPendingActivity();
re-entrancy is not supported.
Hence, disable 'officialPlaybackPosition' updates while
in hasPendingActivity(), as that will trigger v8
allocations by way of microtask allocations.
R=haraken
BUG=
Committed: https://crrev.com/9bf523214cd8788632afb7d20abdb15123ddbc10
Cr-Commit-Position: refs/heads/master@{#432453}
Patch Set 1 #
Total comments: 2
Patch Set 2 : don't roll our own, use AutoReset<> #Messages
Total messages: 25 (17 generated)
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/v2/patch-status/codereview.chromium.or...
sigbjornf@opera.com changed reviewers: + foolip@chromium.org, jochen@chromium.org
please take a look. while peeking at the bots, happened upon https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win7__dbg... Purely functional predicate works better, but this works around the issue, if nothing else.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R= BUG= ========== to ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R= BUG= ==========
sigbjornf@opera.com changed reviewers: + haraken@chromium.org
+haraken
LGTM https://codereview.chromium.org/2498033002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2498033002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3481: DisablePlaybackPositionUpdateScope scope( Can we use AutoReset?
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
https://codereview.chromium.org/2498033002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2498033002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3481: DisablePlaybackPositionUpdateScope scope( On 2016/11/16 06:47:26, haraken wrote: > > Can we use AutoReset? Thanks, I'd forgotten about TemporaryChange<> / AutoReset<> being available.
Dry run: 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R= BUG= ========== to ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R=haraken BUG= ==========
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2498033002/#ps20001 (title: "don't roll our own, use AutoReset<>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R=haraken BUG= ========== to ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R=haraken BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R=haraken BUG= ========== to ========== Media element: avoid v8 allocations in hasPendingActivity(). Blink code is not allowed to allocate objects on the v8 heap while its GC calls out to hasPendingActivity(); re-entrancy is not supported. Hence, disable 'officialPlaybackPosition' updates while in hasPendingActivity(), as that will trigger v8 allocations by way of microtask allocations. R=haraken BUG= Committed: https://crrev.com/9bf523214cd8788632afb7d20abdb15123ddbc10 Cr-Commit-Position: refs/heads/master@{#432453} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9bf523214cd8788632afb7d20abdb15123ddbc10 Cr-Commit-Position: refs/heads/master@{#432453} |
