Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1046123002:
Oilpan: Split completeSweep() in idle tasks into chunks (Closed)
|
Created:
5 years, 8 months ago by haraken Modified:
5 years, 8 months ago CC:
blink-reviews, oilpan-reviews, kouhei+heap_chromium.org, Mads Ager (chromium) Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionOilpan: Split completeSweep() in idle tasks into chunks
Currently completeSweep() is executed in an idle task in one go. This is problematic because it can violate the deadline. This CL splits the completeSweep() into chunks so that each sweeping task meets the deadline.
To avoid the overhead of calling WTF::currentTimeMS() too frequently, we check the current time every time we sweep 10 pages.
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192949
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 7
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 23 (4 generated)
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org, rmcilroy@chromium.org, sigbjornf@opera.com, skyostil@chromium.org
PTAL I'll collect performance numbers from now, but early feedback is welcome :)
As far as I tested in real-world websites manually, the patch set 4 looks working well. Almost all completeSweep()s are complete in one idle task, but I observed a couple of completeSweep()s split into multiple chunks and they are working as expected. PTAL. (Note: I'm planning to land https://codereview.chromium.org/1046123002/ first.)
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { Why 10? https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:652: while (m_firstUnsweptPage) { Nit: Unneeded {}
https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { On 2015/03/31 11:42:07, kouhei wrote: > Why 10? I thought it might be heavy to call Platform::current()->monotonicallyIncreasingTime() at every page sweep (i.e., 128 KB sweep or one LargeObject sweep). If the performance is OK, we can just call it per page. Actually I'm not sure. https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:652: while (m_firstUnsweptPage) { On 2015/03/31 11:42:07, kouhei wrote: > Nit: Unneeded {} Nit: The coding style guide is updated and it's up to us.
lgtm On 2015/03/31 12:46:59, haraken wrote: > https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { > On 2015/03/31 11:42:07, kouhei wrote: > > Why 10? > > I thought it might be heavy to call > Platform::current()->monotonicallyIncreasingTime() at every page sweep (i.e., > 128 KB sweep or one LargeObject sweep). If the performance is OK, we can just > call it per page. Actually I'm not sure. Maybe worth adding a comment? > https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:652: while (m_firstUnsweptPage) { > On 2015/03/31 11:42:07, kouhei wrote: > > Nit: Unneeded {} > > Nit: The coding style guide is updated and it's up to us. Got it.
> https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... > > Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { > > On 2015/03/31 11:42:07, kouhei wrote: > > > Why 10? > > > > I thought it might be heavy to call > > Platform::current()->monotonicallyIncreasingTime() at every page sweep (i.e., > > 128 KB sweep or one LargeObject sweep). If the performance is OK, we can just > > call it per page. Actually I'm not sure. > > Maybe worth adding a comment? Done.
still lgtm
https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { On 2015/03/31 11:42:07, kouhei wrote: > Why 10? Might want to make this a named constant too. Also, maybe it won't happen in practice but you could also check m_firstUnsweptPage != nullptr before scheduling the next sweep (i.e., we ran out of time but also out of pages to sweep).
lgtm with a question. https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { On 2015/03/31 12:52:00, Sami wrote: > On 2015/03/31 11:42:07, kouhei wrote: > > Why 10? > > Might want to make this a named constant too. Also, maybe it won't happen in > practice but you could also check m_firstUnsweptPage != nullptr before > scheduling the next sweep (i.e., we ran out of time but also out of pages to > sweep). +1 https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:640: if (deadlineSeconds <= Platform::current()->monotonicallyIncreasingTime()) { Could we add some slack here (e.g., deadlineSeconds - 0.01 <= currentTime)? Do we have some idea for how much time sweeping a page takes in general?
https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:637: if (pageCount % 10 == 0) { On 2015/03/31 13:01:30, rmcilroy wrote: > On 2015/03/31 12:52:00, Sami wrote: > > On 2015/03/31 11:42:07, kouhei wrote: > > > Why 10? > > > > Might want to make this a named constant too. Also, maybe it won't happen in > > practice but you could also check m_firstUnsweptPage != nullptr before > > scheduling the next sweep (i.e., we ran out of time but also out of pages to > > sweep). > > +1 Will fix. https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:640: if (deadlineSeconds <= Platform::current()->monotonicallyIncreasingTime()) { On 2015/03/31 13:01:30, rmcilroy wrote: > Could we add some slack here (e.g., deadlineSeconds - 0.01 <= currentTime)? Do > we have some idea for how much time sweeping a page takes in general? That is a hard question to answer since the time totally depends on the page contents (it might be just 10 LargeObjects; we can sweep them extremely quickly) and the actual hardware. What value do you recommend for the slack? 0.01 sounds too big to me because the frame budget is less than 0.01 second in most cases, isn't it?
https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:640: if (deadlineSeconds <= Platform::current()->monotonicallyIncreasingTime()) { On 2015/03/31 13:07:45, haraken wrote: > On 2015/03/31 13:01:30, rmcilroy wrote: > > Could we add some slack here (e.g., deadlineSeconds - 0.01 <= currentTime)? Do > > we have some idea for how much time sweeping a page takes in general? > > That is a hard question to answer since the time totally depends on the page > contents (it might be just 10 LargeObjects; we can sweep them extremely quickly) > and the actual hardware. Right I thought this might be the case. > What value do you recommend for the slack? 0.01 sounds too big to me because the > frame budget is less than 0.01 second in most cases, isn't it? Yes that's probably too big. I was thinking of about 1ms or so (0.001 seconds). WDYT? Once this has landed it should be possible to see if the task is blowing the budget in telemetry / tracing (I'm not sure whether you have telemetry measurements which measure the idle time metrics, but you could do something similar to the V8 benchmarks at https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure... if you wanted to track how well you keep to the budget / make use of idle time).
On 2015/03/31 13:19:26, rmcilroy wrote: > https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... > File Source/platform/heap/Heap.cpp (right): > > https://codereview.chromium.org/1046123002/diff/80001/Source/platform/heap/He... > Source/platform/heap/Heap.cpp:640: if (deadlineSeconds <= > Platform::current()->monotonicallyIncreasingTime()) { > On 2015/03/31 13:07:45, haraken wrote: > > On 2015/03/31 13:01:30, rmcilroy wrote: > > > Could we add some slack here (e.g., deadlineSeconds - 0.01 <= currentTime)? > Do > > > we have some idea for how much time sweeping a page takes in general? > > > > That is a hard question to answer since the time totally depends on the page > > contents (it might be just 10 LargeObjects; we can sweep them extremely > quickly) > > and the actual hardware. > > Right I thought this might be the case. > > > What value do you recommend for the slack? 0.01 sounds too big to me because > the > > frame budget is less than 0.01 second in most cases, isn't it? > > Yes that's probably too big. I was thinking of about 1ms or so (0.001 seconds). > WDYT? Once this has landed it should be possible to see if the task is blowing > the budget in telemetry / tracing (I'm not sure whether you have telemetry > measurements which measure the idle time metrics, but you could do something > similar to the V8 benchmarks at > https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure... > if you wanted to track how well you keep to the budget / make use of idle time). Done. Used 0.001 sec. In terms of a risk of violating the deadline, we're doing a much more risky thing when scheduling a marking task. The estimation of the marking task is very rough (i.e., just estimates the marking time based on the collection rate and the marking time of the latest GC), and in fact we observed a couple of cases where the marking task violated the deadline. Compared to the risk of the marking time mis-estimation, I guess the risk of the sweep time mis-estimation won't be an issue.
> Done. Used 0.001 sec. Great, thanks! > In terms of a risk of violating the deadline, we're doing a much more risky > thing when scheduling a marking task. The estimation of the marking task is very > rough (i.e., just estimates the marking time based on the collection rate and > the marking time of the latest GC), and in fact we observed a couple of cases > where the marking task violated the deadline. Compared to the risk of the > marking time mis-estimation, I guess the risk of the sweep time mis-estimation > won't be an issue. Sure I realize this is always estimation and can be off (V8 has similar mis-estimations as well). It would just be good to ensure we measure this to ensure we don't regress (and hopefully improve :) as heuristics change. Happy to help you set up such a benchmark based on the V8 gc_times benchmarks - these have been super useful for us on the V8 idle GC to catch regressions and track improvements.
> Done. Used 0.001 sec. Did you actually do this, it doesn't seem to be in the latest patch-set.
On 2015/03/31 16:01:16, rmcilroy wrote: > > Done. Used 0.001 sec. > > Did you actually do this, it doesn't seem to be in the latest patch-set. Sorry, forgot to upload a CL. Done.
On 2015/03/31 15:59:03, rmcilroy wrote: > > Done. Used 0.001 sec. > > Great, thanks! > > > In terms of a risk of violating the deadline, we're doing a much more risky > > thing when scheduling a marking task. The estimation of the marking task is > very > > rough (i.e., just estimates the marking time based on the collection rate and > > the marking time of the latest GC), and in fact we observed a couple of cases > > where the marking task violated the deadline. Compared to the risk of the > > marking time mis-estimation, I guess the risk of the sweep time mis-estimation > > won't be an issue. > > Sure I realize this is always estimation and can be off (V8 has similar > mis-estimations as well). It would just be good to ensure we measure this to > ensure we don't regress (and hopefully improve :) as heuristics change. Happy > to help you set up such a benchmark based on the V8 gc_times benchmarks - these > have been super useful for us on the V8 idle GC to catch regressions and track > improvements. Yes, we have oilpan_gc_times.py (https://code.google.com/p/chromium/codesearch#chromium/src/tools/perf/measure...). We don't yet have a metric to measure deadline violations, so we need to add them :)
It looks like the CL passed tests. Landing.
The CQ bit was checked by haraken@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/1046123002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1046123002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192949 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
