Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2579773002:
Use WTF::Deque instead of std::queue in the blink scheduler (Closed)
|
Created:
4 years ago by alex clarke (OOO till 29th) Modified:
3 years, 7 months ago CC:
chromium-reviews, blink-reviews, cc-bugs_chromium.org, scheduler-bugs_chromium.org, blink-reviews-wtf_chromium.org, Mikhail Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse WTF::Deque instead of std::queue in the blink scheduler
In theory this will reduce memory wasted by std::deque. It may also help diagnose
some puzzling crashes we've seen related to the immediate incomming and work queues.
BUG=674625
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2579773002
Cr-Commit-Position: refs/heads/master@{#443450}
Committed: https://chromium.googlesource.com/chromium/src/+/63db4c771190c186415af04477809368b0c53e0b
Patch Set 1 #Patch Set 2 : Rebase plus cut down to size #Patch Set 3 : Rebase! #Patch Set 4 : Add some STL naming compatability wrappers to WTF::Deque these all get inlined away by the compiler. #
Total comments: 2
Patch Set 5 : Apply the fix Sami suggested #
Total comments: 5
Messages
Total messages: 45 (34 generated)
Description was changed from ========== WTF::Deque tsan_fix fix comments cut_back big_immediate_change super_dedupe gmm Remover timer tqCompositor scheduler BASE MESSAGE LOOP CANCEL BUG= ========== to ========== WTF::Deque tsan_fix fix comments cut_back big_immediate_change super_dedupe gmm Remover timer tqCompositor scheduler BASE MESSAGE LOOP CANCEL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== WTF::Deque tsan_fix fix comments cut_back big_immediate_change super_dedupe gmm Remover timer tqCompositor scheduler BASE MESSAGE LOOP CANCEL BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625, 674895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
alexclarke@chromium.org changed reviewers: + haraken@chromium.org, skyostil@chromium.org
Description was changed from ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625, 674895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625, 674895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@chromium.org 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm -- but added one observation. https://codereview.chromium.org/2579773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Deque.h (right): https://codereview.chromium.org/2579773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:532: if (m_end == m_buffer.capacity() - 1) Looks like this already exists with the current code, but I'm wondering if we should move this adjustment before the construction? I'm imagining a case where the constructor also happens to touch this same deque. Maybe that's something for a different patch.
https://codereview.chromium.org/2579773002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Deque.h (right): https://codereview.chromium.org/2579773002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:532: if (m_end == m_buffer.capacity() - 1) On 2017/01/11 13:49:11, Sami wrote: > Looks like this already exists with the current code, but I'm wondering if we > should move this adjustment before the construction? I'm imagining a case where > the constructor also happens to touch this same deque. Maybe that's something > for a different patch. Done.
The CQ bit was checked by alexclarke@chromium.org 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...
haraken@chromium.org changed reviewers: + pilgrim@chromium.org
LGTM with a comment. https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Deque.h (right): https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:156: void emplace_front(Args&&...); pilgrim@: Does this align with your renaming plan? https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:549: } Can we share these implementations with append/prepend?
https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Deque.h (right): https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:156: void emplace_front(Args&&...); On 2017/01/11 15:20:35, haraken wrote: > > pilgrim@: Does this align with your renaming plan? It might make sense to (in a follow up patch) rename the blink uses to conform to STL style. Also I only checked the things I used, possible I missed some. https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:549: } On 2017/01/11 15:20:35, haraken wrote: > > Can we share these implementations with append/prepend? Do you mean add emplace_append and emplace_prepend? I can certainly add those if that's what you mean :)
https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/wtf/Deque.h (right): https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/wtf/Deque.h:549: } On 2017/01/11 15:29:53, alex clarke wrote: > On 2017/01/11 15:20:35, haraken wrote: > > > > Can we share these implementations with append/prepend? > > Do you mean add emplace_append and emplace_prepend? I can certainly add those > if that's what you mean :) Ah, sorry. Ignore my comment :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/11 15:20:35, haraken wrote: > LGTM with a comment. > > https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/wtf/Deque.h (right): > > https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/Deque.h:156: void emplace_front(Args&&...); > > pilgrim@: Does this align with your renaming plan? Should I wait for pilgrim@ or can we just land this and fix it later? It seems to be a win on the perf bot as far as memory goes. > > https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/wtf/Deque.h:549: } > > Can we share these implementations with append/prepend?
On 2017/01/12 at 17:39:06, alexclarke wrote: > On 2017/01/11 15:20:35, haraken wrote: > > LGTM with a comment. > > > > https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/wtf/Deque.h (right): > > > > https://codereview.chromium.org/2579773002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/wtf/Deque.h:156: void emplace_front(Args&&...); > > > > pilgrim@: Does this align with your renaming plan? > > Should I wait for pilgrim@ or can we just land this and fix it later? It seems to be a win on the perf bot as far as memory goes. > Hi, sorry, please land this.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2579773002/#ps80001 (title: "Apply the fix Sami suggested")
Thanks all!
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": 80001, "attempt_start_ts": 1484251232366260,
"parent_rev": "8c06f31ff74dea57b5eab5f2f5ef2f407f71ae97", "commit_rev":
"63db4c771190c186415af04477809368b0c53e0b"}
Message was sent while issue was closed.
Description was changed from ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625, 674895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625, 674895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2579773002 Cr-Commit-Position: refs/heads/master@{#443450} Committed: https://chromium.googlesource.com/chromium/src/+/63db4c771190c186415af0447780... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/63db4c771190c186415af0447780...
Message was sent while issue was closed.
Description was changed from ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625, 674895 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2579773002 Cr-Commit-Position: refs/heads/master@{#443450} Committed: https://chromium.googlesource.com/chromium/src/+/63db4c771190c186415af0447780... ========== to ========== Use WTF::Deque instead of std::queue in the blink scheduler In theory this will reduce memory wasted by std::deque. It may also help diagnose some puzzling crashes we've seen related to the immediate incomming and work queues. BUG=674625 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2579773002 Cr-Commit-Position: refs/heads/master@{#443450} Committed: https://chromium.googlesource.com/chromium/src/+/63db4c771190c186415af0447780... ==========
Message was sent while issue was closed.
On 2017/01/13 01:49:34, commit-bot: I haz the power wrote: > Committed patchset #5 (id:80001) as > https://chromium.googlesource.com/chromium/src/+/63db4c771190c186415af0447780... Hi, this change is causing crash in blink_platform_perftests on Ubuntu 16.04. More information is at: https://bugs.chromium.org/p/chromium/issues/detail?id=718894 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
