[heap] Ensure heap limit is above current size [v8/v8 : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Related details
- Dominik Inführ
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
- Editor-CodeMirror: base median = 20.484999999776484 -> patched median = 20.560000000149014
- NewsSite-Nuxt: base median = 64.74999999981374 -> patched median = 65.14499999843538
- TodoMVC-JavaScript-ES5: base median = 44.530000000633294 -> patched median = 44.62000000011176
- TodoMVC-WebComponents: base median = 17.04500000029802 -> patched median = 17.139999999292193
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/109c28e7f10000
Related details
- Dominik Inführ
- Etienne Pierre-Doray
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2.crossbench complete.
- stanford-crypto-pbkdf2: base median = 1243.2887518742516 -> patched median = 1242.2197154469584
- typescript: base median = 41.858721393205094 -> patched median = 41.80884780655206
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17a641bff10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
- Editor-CodeMirror: base median = 20.539999999478457 -> patched median = 20.619999998807906
- NewsSite-Nuxt: base median = 64.80999999903142 -> patched median = 65.00000000074506
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/101724a4090000
Dominik Inführ (Gerrit)
Dominik Inführ added 2 comments![]( "Open in Gerrit")
static constexpr size_t kMinimumAllocationLimitGrowingStep = 64 * MB;(If we decided to keep this method as-is) Nit: Can we drop this method and just define the constant for GlobalMemoryTrait instead?
return base::saturated_cast<size_t>(std::max(Do I understand correctly that 64M is now our minimum allocation limit growing step? While I like the simplicity of it, it seems a bit high.
Related details
- Etienne Pierre-Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 3 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
3 comments
static constexpr size_t kMinimumAllocationLimitGrowingStep = 64 * MB;(If we decided to keep this method as-is) Nit: Can we drop this method and just define the constant for GlobalMemoryTrait instead?
Done
The V8HeapTrait counterpart could do the same in the follow-up
return base::saturated_cast<size_t>(std::max(Do I understand correctly that 64M is now our minimum allocation limit growing step? While I like the simplicity of it, it seems a bit high.
Do I understand correctly that 64M is now our minimum allocation limit growing step?
Yes
While I like the simplicity of it, it seems a bit high.
This includes external memory though (hence the 64M). I suppose if there's no external memory, we're effectively giving oilpan a larger min growing step which may be undesirable.
Looking at ukm data, hitting the 8mb growing step is quite common.
We could keep the min = 8mb (done in latest patchset), which should have no impact in most cases, at the cost of more frequent GCs when above 8Gb; this still serves the goal of unblocking external_memory_accounted_in_global_limit.
Related details
- Dominik Inführ
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/jetstream2.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17029400090000
Related details
- Dominik Inführ
- Etienne Pierre-Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/145928e7f10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
- Score: base median = 30.019634504624328 -> patched median = 30.00076848203188
- TodoMVC-Backbone: base median = 26 -> patched median = 26.15999999977648
- TodoMVC-JavaScript-ES5: base median = 44.77999999970198 -> patched median = 45.09499999955297
- TodoMVC-JavaScript-ES6-Webpack-Complex-DOM: base median = 38.74999999962747 -> patched median = 39.03500000163913
- TodoMVC-Svelte-Complex-DOM: base median = 11.325 -> patched median = 11.370000001043081
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16afe186090000
Related details
- Dominik Inführ
- Etienne Pierre-Doray
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1490811c090000
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
return base::saturated_cast<size_t>(std::max(Etienne Pierre-DorayDo I understand correctly that 64M is now our minimum allocation limit growing step? While I like the simplicity of it, it seems a bit high.
Do I understand correctly that 64M is now our minimum allocation limit growing step?
Yes
While I like the simplicity of it, it seems a bit high.
This includes external memory though (hence the 64M). I suppose if there's no external memory, we're effectively giving oilpan a larger min growing step which may be undesirable.
Looking at ukm data, hitting the 8mb growing step is quite common.
We could keep the min = 8mb (done in latest patchset), which should have no impact in most cases, at the cost of more frequent GCs when above 8Gb; this still serves the goal of unblocking external_memory_accounted_in_global_limit.
I realize that using 8Mb breaks Regress9701 again when enabling external_memory_accounted_in_global_limit.
But then I also realize that Regress9701 only succeeds ToT because the test allocates 64000Kb, which is below 64Mb. Simply increasing the total size of allocations breaks the test:
https://chromium-review.git.corp.google.com/c/v8/v8/+/7653295?tab=checks
So to unblock external_memory_accounted_in_global_limit, I think we should just disable the test until I find a better solution, or a write a better test.
AFAIU what we really might want is to ensure scavenger runs before full GC when allocating AB if that makes a meaningful difference (it doesn't really matter on an empty heap), but that's a bit out of scope.
Related details
- Dominik Inführ
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
1 comment
Related details
- Etienne Pierre-Doray
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[heap] Ensure heap limit is above current size
The assumption that allocation_limit < max_size isn't always true.
This won't hold if current size > max_size.
When this happens for old generation limit, we'll eventually crash, but that's not the case for global limit.
This CL makes sure that BoundAllocationLimit returns a value above
current_size for global limit, to avoid repeated GCs.
A minimum step of 8Mb is chosen, which match the existing min growing
step;
this follows a similar behavior of allowing a minimum amount of growth
before the next GC (but this is only relevant when above max
global size).
- M src/heap/heap-controller.cc
- M src/heap/heap-controller.h
- M test/cctest/heap/test-heap.cc
- M test/mjsunit/mjsunit.status
- M test/unittests/heap/heap-controller-unittest.cc
Code-Review: +1 by Dominik Inführ
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |