Add input hint [v8/v8 : main]
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 2 comments![]( "Open in Gerrit")
perfetto::DynamicString track_name,Thiabaud EngelbrechtUse perfetto::StaticString instead.
Also you could pass perfetto::NamedTrack directly as argument, and let the caller create the track.
Done
TRACE_EVENT_BEGIN(TRACE_DISABLED_BY_DEFAULT("v8.gc"), "IsLoading", track_);Thiabaud EngelbrechtRenamed to either "Active" or plumb the appropriate name from Heap as a StaticString.
Done
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. |
Benoit Lize (Gerrit)
Benoit Lize added 4 comments![]( "Open in Gerrit")
High level comments:
Right now the implementation of loading and input are shared, but this is likely not what we want: one if for throughput, the other one for responsiveness.
For instance, we may not want to change GC limits for input, just to temporarily waive the limit (up to a point), whereas the reset logic for limit for loading would be different (since we expect the live heap to grow, and a lot of garbage to be generated).
Also we need to handle stacking of the two hints.
So I would suggest:
- Inline the conditions like "IsLoading() and IsInputHandling()" in the calling code
- Make sure that they stack correctly
Also, is it possible to add tests? In particular, can you test the stacking? So something like input while loading, and weird overlaps like that.
class HighResponsivenessModeState {nit: what about naming this something like GCHintState or something?
And then you can make use an enum for the two types of hints:
enum class GCHintType {
kNone,
kLoading,
kInputHandling
} bool IsHighResponsivenessMode() const {nitpicking: this is not really about responsiveness in both cases, this is about:
- Throughput for loading
- Main thread responsiveness for input.
Right now, they resolve to the same implementation, but this is unlikely to be the optimal one. So perhaps do not add something here, and call out the use cases specifically inline in the code?
static const int kMaxInputHandlingTimeMs = 3000;Add a comment explaining why this delay.
In particular I think you need to explain that there is a max in case the client never resets the flag, and that "input" can actually be long running, for instance scrolling.
Related details
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
High level comments:
Right now the implementation of loading and input are shared, but this is likely not what we want: one if for throughput, the other one for responsiveness.For instance, we may not want to change GC limits for input, just to temporarily waive the limit (up to a point), whereas the reset logic for limit for loading would be different (since we expect the live heap to grow, and a lot of garbage to be generated).
Also we need to handle stacking of the two hints.
So I would suggest:
- Inline the conditions like "IsLoading() and IsInputHandling()" in the calling code
- Make sure that they stack correctly
Also, is it possible to add tests? In particular, can you test the stacking? So something like input while loading, and weird overlaps like that.
I requested a design doc for this particular project that asked for:
1. splitting of the signal (which is done)
2. finding memory limits that are reasonable for for delaying GC (as loading is effectively disabling it for most parts) (wip)
3. Figuring out returning after the "scope" was left. Stacking is triggering similar cases. (wip)
I think the CL is going in the right direction but given your comments/questions here Benoit (and the fact that I want to see very similar things), I wonder if we shouldn't pause it until these questions are answered on a design doc.
Related details
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 3 comments![]( "Open in Gerrit")
nit: what about naming this something like GCHintState or something?
And then you can make use an enum for the two types of hints:
enum class GCHintType {
kNone,
kLoading,
kInputHandling
}
Done
nitpicking: this is not really about responsiveness in both cases, this is about:
- Throughput for loading
- Main thread responsiveness for input.
Right now, they resolve to the same implementation, but this is unlikely to be the optimal one. So perhaps do not add something here, and call out the use cases specifically inline in the code?
Done
Add a comment explaining why this delay.
In particular I think you need to explain that there is a max in case the client never resets the flag, and that "input" can actually be long running, for instance scrolling.
- Benoit Lize
- 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 added 5 comments![]( "Open in Gerrit")
update_allocation_limits_after_loading_ = true;Since the exit code is different, I think we need a update_allocation_limits_after_input_ instead.
RecomputeLimitsAfterLoadingIfNeeded();We'll need to call RecomputeLimitsAfterInputHandlingIfNeeded() as well; this is what prevents us from hitting the overshoot limit.
void Heap::RecomputeLimitsAfterInputHandlingIfNeeded() {Similar to update_allocation_limits_after_loading_, I think we need a update_allocation_limits_after_input_ here (to honour the "IfNeeded" part). The "recompute" happens:
- at the beginning of incremental marking
- when exiting loading/input state
Why not just look at IsLoading/IsInputHandling? AFAIU, this would work if we rely on timeout, but since we might not get the end notification, we want to make the update_allocation_limits_after_* persistent if the state changes without a notification.
if (!v8_flags.external_memory_accounted_in_global_limit) {
size_now += AllocatedExternalMemorySinceMarkCompact();
}Let's not include this; this is essentially a bug but I kept the behavior in AllocationLimitOvershotByLargeMargin to experiement.
if (!heap->IsLoading() && !heap->IsInputHandling()) {
(heap->*callback_)();
// heap->RecomputeLimitsAfterHighResponsivenessModeIfNeeded();
if (auto* job = heap->incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already started)
// and advance marking if incremental marking is active.
job->ScheduleTask();
}
}High-level comment: Effectively I believe the desired behavior is:
- when exiting input, call RecomputeLimitsAfterInputHandlingIfNeeded if !IsLoading()
- when exiting loading, call RecomputeLimitsAfterLoadingIfNeeded
Since we have 2 different calls and different condidtions, I don't think we gain anything from placing this in GCHintState. It seems i'd be simpler if called directly from NotifyInputHandlingEnded() and NotifyLoadingEnded()
Related details
- Benoit Lize
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
- Benoit Lize
- Dominik Inführ
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 5 comments![]( "Open in Gerrit")
update_allocation_limits_after_loading_ = true;Since the exit code is different, I think we need a update_allocation_limits_after_input_ instead.
Done
We'll need to call RecomputeLimitsAfterInputHandlingIfNeeded() as well; this is what prevents us from hitting the overshoot limit.
Done
void Heap::RecomputeLimitsAfterInputHandlingIfNeeded() {Similar to update_allocation_limits_after_loading_, I think we need a update_allocation_limits_after_input_ here (to honour the "IfNeeded" part). The "recompute" happens:
- at the beginning of incremental marking
- when exiting loading/input state
Why not just look at IsLoading/IsInputHandling? AFAIU, this would work if we rely on timeout, but since we might not get the end notification, we want to make the update_allocation_limits_after_* persistent if the state changes without a notification.
Done
if (!v8_flags.external_memory_accounted_in_global_limit) {
size_now += AllocatedExternalMemorySinceMarkCompact();
}Let's not include this; this is essentially a bug but I kept the behavior in AllocationLimitOvershotByLargeMargin to experiement.
Done
if (!heap->IsLoading() && !heap->IsInputHandling()) {
(heap->*callback_)();
// heap->RecomputeLimitsAfterHighResponsivenessModeIfNeeded();
if (auto* job = heap->incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already started)
// and advance marking if incremental marking is active.
job->ScheduleTask();
}
}High-level comment: Effectively I believe the desired behavior is:
- when exiting input, call RecomputeLimitsAfterInputHandlingIfNeeded if !IsLoading()
- when exiting loading, call RecomputeLimitsAfterLoadingIfNeeded
Since we have 2 different calls and different condidtions, I don't think we gain anything from placing this in GCHintState. It seems i'd be simpler if called directly from NotifyInputHandlingEnded() and NotifyLoadingEnded()
- Benoit Lize
- 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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 4 comments![]( "Open in Gerrit")
Let's add documentation on this method.
if (!IsLoading() && !IsInputHandling()) {I think we should do this even if IsInputHandling() == true.
`IsLoading()` should always be true.
So RecomputeLimitsAfterLoadingIfNeeded() can be called unconditionally.
if (!IsLoading() && !IsInputHandling()) {I think just `IsLoading()` is sufficient, `!IsInputHandling()` should always be true here.
void Heap::NotifyLoadingEnded() { loading_state_.NotifyEnded(this); }I don't think we gain in code reuse going through `callback_`. How about:
```
void Heap::NotifyLoadingEnded() {
loading_state_.NotifyEnded(this);
RecomputeLimitsAfterLoadingIfNeeded();
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already started)
// and advance marking if incremental marking is active.
job->ScheduleTask();
}
}
void Heap::NotifyInputHandlingEnded() {
input_handling_state_.NotifyEnded(this);
if (IsLoading()) {
// loading state is more aggressive than input state.
return;
}
RecomputeLimitsAfterInputHandlingIfNeeded();
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already started)
// and advance marking if incremental marking is active.
job->ScheduleTask();
}
}
void Heap::GCHintState::NotifyEnded(Heap* heap) {
start_time_ms_.store(kInactive, std::memory_order_relaxed);
TRACE_EVENT_END(TRACE_DISABLED_BY_DEFAULT("v8.gc"), track_);
}```
Related details
- Benoit Lize
- Dominik Inführ
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 4 comments![]( "Open in Gerrit")
Let's add documentation on this method.
Done
I think we should do this even if IsInputHandling() == true.
`IsLoading()` should always be true.
So RecomputeLimitsAfterLoadingIfNeeded() can be called unconditionally.
Done
I think just `IsLoading()` is sufficient, `!IsInputHandling()` should always be true here.
Done
void Heap::NotifyLoadingEnded() { loading_state_.NotifyEnded(this); }I don't think we gain in code reuse going through `callback_`. How about:
```
void Heap::NotifyLoadingEnded() {
loading_state_.NotifyEnded(this);
RecomputeLimitsAfterLoadingIfNeeded();
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already started)
// and advance marking if incremental marking is active.
job->ScheduleTask();
}
}void Heap::NotifyInputHandlingEnded() {
input_handling_state_.NotifyEnded(this);
if (IsLoading()) {
// loading state is more aggressive than input state.
return;
}
RecomputeLimitsAfterInputHandlingIfNeeded();
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already started)
// and advance marking if incremental marking is active.
job->ScheduleTask();
}
}void Heap::GCHintState::NotifyEnded(Heap* heap) {
start_time_ms_.store(kInactive, std::memory_order_relaxed);
TRACE_EVENT_END(TRACE_DISABLED_BY_DEFAULT("v8.gc"), track_);
}```
- 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
| Code-Review | +1 |
3 comments
LGTM (please wait for dinfuehr review)
void Heap::NotifyInputHandlingStarted() {
input_handling_state_.NotifyStarted(this);
}
void Heap::NotifyInputHandlingEnded() {Do you want to add a flag in v8 for this? I think here would be a good place to check the flag (or maybe in SetIsInputHandling()).
Related details
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 2 comments![]( "Open in Gerrit")
Add input hintThiabaud EngelbrechtNit: add [heap] tag
Done
void Heap::NotifyInputHandlingStarted() {
input_handling_state_.NotifyStarted(this);
}
void Heap::NotifyInputHandlingEnded() {Do you want to add a flag in v8 for this? I think here would be a good place to check the flag (or maybe in SetIsInputHandling()).
- 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. |
Benoit Lize (Gerrit)
Benoit Lize added 7 comments![]( "Open in Gerrit")
Per offline conversation, looks good overall, can you add tests as well?
"how much we are allowed to overshoot the heap limit by during "Need to state the unit here, MiB?
// bool update_allocation_limits_after_loading_ = false;nit: remove
const double kMaxTimeMs;nit: should not use this case if not constexpr.
const double max_time_;
std::atomic<double> start_time_ms_{kInactive};Use v8::base::TimeTicks and v8::base::TimeDelta here and below
DCHECK(fixed_margin <= 64);Why the DCHECK()?
If you want to protect against very high values, perhaps std::clamp() instead?
bool Heap::AllocationLimitOvershotByFixedMargin(Can the code be merged with the one above?
In the case above the margin is a percentage, but it gets computed before applying the limits.
Related details
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
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 added 1 comment![]( "Open in Gerrit")
bool Heap::AllocationLimitOvershotByFixedMargin(Can the code be merged with the one above?
In the case above the margin is a percentage, but it gets computed before applying the limits.
Come to think of it, we probably want to apply the "or half-way to the max heap" bit from above, or simply also call `|| AllocationLimitOvershotByLargeMargin()`
+1 to merging if possible.
Related details
- Thiabaud Engelbrecht
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 5 comments![]( "Open in Gerrit")
"how much we are allowed to overshoot the heap limit by during "Need to state the unit here, MiB?
Done
// bool update_allocation_limits_after_loading_ = false;Thiabaud Engelbrechtnit: remove
Done
nit: should not use this case if not constexpr.
const double max_time_;
Done
Why the DCHECK()?
If you want to protect against very high values, perhaps std::clamp() instead?
Done
Etienne Pierre-DorayCan the code be merged with the one above?
In the case above the margin is a percentage, but it gets computed before applying the limits.
Come to think of it, we probably want to apply the "or half-way to the max heap" bit from above, or simply also call `|| AllocationLimitOvershotByLargeMargin()`
+1 to merging if possible.
Didn't merge since it's only a few lines, but called the other function too.
Related details
- Benoit Lize
- 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. |
Dominik Inführ (Gerrit)
Dominik Inführ added 3 comments![]( "Open in Gerrit")
// Chrome.Nit: Can we add the path + codesearch link here to the Chromium constant?
if ((collector == GarbageCollector::MARK_COMPACTOR)) {Nit: ((double parens))
RecomputeLimitsAfterLoadingIfNeeded();We now have two methods here that conditionally update limits such that we do not finalize incremental marking immediately afterwards. What about simply doing that unconditionally simply because we never really want. E.g. a method RecomputeLimitsOnIncrementalMarkingStart(). That could maybe. simplify the use of the update_allocation_limits_after_ended_.
Related details
- Thiabaud Engelbrecht
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 added 1 comment![]( "Open in Gerrit")
RecomputeLimitsAfterLoadingIfNeeded();We now have two methods here that conditionally update limits such that we do not finalize incremental marking immediately afterwards. What about simply doing that unconditionally simply because we never really want. E.g. a method RecomputeLimitsOnIncrementalMarkingStart(). That could maybe. simplify the use of the update_allocation_limits_after_ended_.
What about simply doing that unconditionally simply because we never really want.
I think that's what I wanted to try here?
https://chromium-review.googlesource.com/c/v8/v8/+/7004685/19/src/heap/heap.cc
(in the scope of b/454925310)
+1 for doing this, but this comes with a greater risk of "getting behind"
Related details
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
SetOldGenerationAndGlobalAllocationLimit(Could we be more uniform here with RecomputeLimitsAfterLoadingIfNeeded(). For example in that method we use `UpdateAllocationLimits` instead of `SetOldGenerationAndGlobalAllocationLimit`. Also we don't set those fields here (not sure whether necessary). We also use `OldGenerationSpaceAvailable()` there. Could we align all of that? It's quite tricky to understand/make sense of/remember all the possible subtle differences in the approaches here.
Related details
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 3 comments![]( "Open in Gerrit")
static const int kMaxInputHandlingTimeMs = 3000;How often and long is the input mode active? With the current implementation couldn't we run into situations where we (through entering input mode constantly) never trigger a GC because we constantly recompute and increase limits in NotifyInputHandlingEnded() until we eventually fail to allocate?
SetOldGenerationAndGlobalAllocationLimit(Could we be more uniform here with RecomputeLimitsAfterLoadingIfNeeded(). For example in that method we use `UpdateAllocationLimits` instead of `SetOldGenerationAndGlobalAllocationLimit`. Also we don't set those fields here (not sure whether necessary). We also use `OldGenerationSpaceAvailable()` there. Could we align all of that? It's quite tricky to understand/make sense of/remember all the possible subtle differences in the approaches here.
I've talked to Omer and Michael about this. Maybe we can use this "reset limits" approach for loading as well, so that we unify both modes based on this approach. I will try to write a CL for this and report back.
return 1024 * 1024 * fixed_margin;Nit: Use MB here.
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
SetOldGenerationAndGlobalAllocationLimit(Dominik InführCould we be more uniform here with RecomputeLimitsAfterLoadingIfNeeded(). For example in that method we use `UpdateAllocationLimits` instead of `SetOldGenerationAndGlobalAllocationLimit`. Also we don't set those fields here (not sure whether necessary). We also use `OldGenerationSpaceAvailable()` there. Could we align all of that? It's quite tricky to understand/make sense of/remember all the possible subtle differences in the approaches here.
I've talked to Omer and Michael about this. Maybe we can use this "reset limits" approach for loading as well, so that we unify both modes based on this approach. I will try to write a CL for this and report back.
The motivation for the 2 RecomputeLimitsAfter* methods was to isolate behavior change to only input related signal (and leave loading untouched), and attempt to change the loading as a follow-up.
Overall, I'm in favour of using this "reset limits" logic everywhere, but that means the experiment will have a scope a bit beyond input hint (with possibly effects that are hard to understand).
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 5 comments![]( "Open in Gerrit")
Use v8::base::TimeTicks and v8::base::TimeDelta here and below
Acknowledged. Will do in a follow-up CL.
if ((collector == GarbageCollector::MARK_COMPACTOR)) {Thiabaud EngelbrechtNit: ((double parens))
Done
Etienne Pierre-DorayWe now have two methods here that conditionally update limits such that we do not finalize incremental marking immediately afterwards. What about simply doing that unconditionally simply because we never really want. E.g. a method RecomputeLimitsOnIncrementalMarkingStart(). That could maybe. simplify the use of the update_allocation_limits_after_ended_.
What about simply doing that unconditionally simply because we never really want.
I think that's what I wanted to try here?
https://chromium-review.googlesource.com/c/v8/v8/+/7004685/19/src/heap/heap.cc
(in the scope of b/454925310)
+1 for doing this, but this comes with a greater risk of "getting behind"
Done
Dominik InführCould we be more uniform here with RecomputeLimitsAfterLoadingIfNeeded(). For example in that method we use `UpdateAllocationLimits` instead of `SetOldGenerationAndGlobalAllocationLimit`. Also we don't set those fields here (not sure whether necessary). We also use `OldGenerationSpaceAvailable()` there. Could we align all of that? It's quite tricky to understand/make sense of/remember all the possible subtle differences in the approaches here.
Etienne Pierre-DorayI've talked to Omer and Michael about this. Maybe we can use this "reset limits" approach for loading as well, so that we unify both modes based on this approach. I will try to write a CL for this and report back.
The motivation for the 2 RecomputeLimitsAfter* methods was to isolate behavior change to only input related signal (and leave loading untouched), and attempt to change the loading as a follow-up.
Overall, I'm in favour of using this "reset limits" logic everywhere, but that means the experiment will have a scope a bit beyond input hint (with possibly effects that are hard to understand).
Acknowledged
return 1024 * 1024 * fixed_margin;Thiabaud EngelbrechtNit: Use MB here.
Done
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. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
void Heap::RecomputeLimitsAfterInputHandlingIfNeeded() {Let's just drop this method and use `EnsureAllocationLimitAboveCurrentSize()` directly. We want to get rid of the corresponding method for loading as well.
Related details
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 5 comments![]( "Open in Gerrit")
This is already looking good.
Added a few comments + Please address Dominik's open comments.
uint64_t Heap::GetFixedMarginForInputHandlingBytes() const {Let's make this a free standing method in an anonymous namespace. I don't see a reason for it to be a part of `Heap`.
DCHECK(v8_flags.optimize_for_input_handling);I think if this method assumes it's only ever used for input handling, we don't need to get the `overshoot_margin` as a parameter. We can just hardcode a call to `GetFixedMarginForInputHandlingBytes()`.
If we want this method to be more generic and accept various margins, I think we should the DCHECK here. I'm leaning towards this option.
return IsInputHandling() && !(AllocationLimitOvershotByFixedMargin(If `optimize_for_input_handling` is false, is there a reason to even initialize `input_handling_state_`? Practically, if never initialize `input_handling_state_` and if the flag is false, `IsInputHandling()` will subsume the flag check above.
input_handling_state_.NotifyEnded(this);nit: Can we move this to the end of method just to align with `NotifyLoadingEnded`?
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 7 comments![]( "Open in Gerrit")
static const int kMaxInputHandlingTimeMs = 3000;How often and long is the input mode active? With the current implementation couldn't we run into situations where we (through entering input mode constantly) never trigger a GC because we constantly recompute and increase limits in NotifyInputHandlingEnded() until we eventually fail to allocate?
Should be active whenever the user is interacting with Chrome. Which shouldn't be too often/too long (as seen by the results of the experiment we ran already).
I believe the check with |AllocationLimitOvershotByLargeMargin()| below should avoid this case though (please correct me if I'm wrong here).
// Chrome.Thiabaud EngelbrechtNit: Can we add the path + codesearch link here to the Chromium constant?
Acknowledged
void Heap::RecomputeLimitsAfterInputHandlingIfNeeded() {Let's just drop this method and use `EnsureAllocationLimitAboveCurrentSize()` directly. We want to get rid of the corresponding method for loading as well.
I'd like to keep the DCHECK I have here, and remove both methods at the same later on? That way I can ensure that we only call this in the experiment group for now.
uint64_t Heap::GetFixedMarginForInputHandlingBytes() const {Let's make this a free standing method in an anonymous namespace. I don't see a reason for it to be a part of `Heap`.
Done
I think if this method assumes it's only ever used for input handling, we don't need to get the `overshoot_margin` as a parameter. We can just hardcode a call to `GetFixedMarginForInputHandlingBytes()`.
If we want this method to be more generic and accept various margins, I think we should the DCHECK here. I'm leaning towards this option.
Acknowledged
return IsInputHandling() && !(AllocationLimitOvershotByFixedMargin(If `optimize_for_input_handling` is false, is there a reason to even initialize `input_handling_state_`? Practically, if never initialize `input_handling_state_` and if the flag is false, `IsInputHandling()` will subsume the flag check above.
I'd like to add a follow-up CL with metrics. As part of that, I'd like to know if we were in the input state or not, so I've made sure we track that properly in this CL,regardless of if the feature is enabled or not.
nit: Can we move this to the end of method just to align with `NotifyLoadingEnded`?
Done (moved loading instead because we have the early return here)
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 4 comments![]( "Open in Gerrit")
static const int kMaxInputHandlingTimeMs = 3000;Thiabaud EngelbrechtHow often and long is the input mode active? With the current implementation couldn't we run into situations where we (through entering input mode constantly) never trigger a GC because we constantly recompute and increase limits in NotifyInputHandlingEnded() until we eventually fail to allocate?
Should be active whenever the user is interacting with Chrome. Which shouldn't be too often/too long (as seen by the results of the experiment we ran already).
I believe the check with |AllocationLimitOvershotByLargeMargin()| below should avoid this case though (please correct me if I'm wrong here).
I think Dominik's concern was that if you exit and enter input mode repetitively, each time you exit you bump the limits. Potentially that means you may never hit the overshot by large margin case.
I initially thought that scheduling the incremental marking job is what protects us from this. However if the renderer is busy handling input, we won't get that task either. Likely there is a way to misuse input mode to continuously grow the limits.
I think this problem also exists with the current loading mode if it's misused, but I suspect the intended usage of input mode makes it more likely to accidentally be misused. E.g. if a user keeps scrolling the page up and down, will we be forever in input mode? My understanding is that the answer is yes.
This needs a mitigation, but for the sake of progress we can also add a TODO for now, land without a mitigation, and add one in a followup (before we start experimenting with it imo).
void Heap::RecomputeLimitsAfterInputHandlingIfNeeded() {Thiabaud EngelbrechtLet's just drop this method and use `EnsureAllocationLimitAboveCurrentSize()` directly. We want to get rid of the corresponding method for loading as well.
I'd like to keep the DCHECK I have here, and remove both methods at the same later on? That way I can ensure that we only call this in the experiment group for now.
Acknowledged
DCHECK(v8_flags.optimize_for_input_handling);Thiabaud EngelbrechtI think if this method assumes it's only ever used for input handling, we don't need to get the `overshoot_margin` as a parameter. We can just hardcode a call to `GetFixedMarginForInputHandlingBytes()`.
If we want this method to be more generic and accept various margins, I think we should the DCHECK here. I'm leaning towards this option.
Acknowledged
Can we already update this method to either be input mode specific or generic?
return IsInputHandling() && !(AllocationLimitOvershotByFixedMargin(Thiabaud EngelbrechtIf `optimize_for_input_handling` is false, is there a reason to even initialize `input_handling_state_`? Practically, if never initialize `input_handling_state_` and if the flag is false, `IsInputHandling()` will subsume the flag check above.
I'd like to add a follow-up CL with metrics. As part of that, I'd like to know if we were in the input state or not, so I've made sure we track that properly in this CL,regardless of if the feature is enabled or not.
I see. Keep in mind that means that if there are any regressions coming in from this CL, we can't argue it's all behind a flag.
Can you add a TODO to consider not entering input mode at all when the flag is off?
That way if the metrics CL never lands, we have documentation that we can simplify the code here.
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
I think one last revision should suffice and then we can land. Thanks
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 3 comments![]( "Open in Gerrit")
static const int kMaxInputHandlingTimeMs = 3000;Thiabaud EngelbrechtHow often and long is the input mode active? With the current implementation couldn't we run into situations where we (through entering input mode constantly) never trigger a GC because we constantly recompute and increase limits in NotifyInputHandlingEnded() until we eventually fail to allocate?
Omer KatzShould be active whenever the user is interacting with Chrome. Which shouldn't be too often/too long (as seen by the results of the experiment we ran already).
I believe the check with |AllocationLimitOvershotByLargeMargin()| below should avoid this case though (please correct me if I'm wrong here).
I think Dominik's concern was that if you exit and enter input mode repetitively, each time you exit you bump the limits. Potentially that means you may never hit the overshot by large margin case.
I initially thought that scheduling the incremental marking job is what protects us from this. However if the renderer is busy handling input, we won't get that task either. Likely there is a way to misuse input mode to continuously grow the limits.
I think this problem also exists with the current loading mode if it's misused, but I suspect the intended usage of input mode makes it more likely to accidentally be misused. E.g. if a user keeps scrolling the page up and down, will we be forever in input mode? My understanding is that the answer is yes.
This needs a mitigation, but for the sake of progress we can also add a TODO for now, land without a mitigation, and add one in a followup (before we start experimenting with it imo).
Done
DCHECK(v8_flags.optimize_for_input_handling);I think if this method assumes it's only ever used for input handling, we don't need to get the `overshoot_margin` as a parameter. We can just hardcode a call to `GetFixedMarginForInputHandlingBytes()`.
If we want this method to be more generic and accept various margins, I think we should the DCHECK here. I'm leaning towards this option.
Omer KatzAcknowledged
Can we already update this method to either be input mode specific or generic?
Done
return IsInputHandling() && !(AllocationLimitOvershotByFixedMargin(Thiabaud EngelbrechtIf `optimize_for_input_handling` is false, is there a reason to even initialize `input_handling_state_`? Practically, if never initialize `input_handling_state_` and if the flag is false, `IsInputHandling()` will subsume the flag check above.
Omer KatzI'd like to add a follow-up CL with metrics. As part of that, I'd like to know if we were in the input state or not, so I've made sure we track that properly in this CL,regardless of if the feature is enabled or not.
I see. Keep in mind that means that if there are any regressions coming in from this CL, we can't argue it's all behind a flag.
Can you add a TODO to consider not entering input mode at all when the flag is off?
That way if the metrics CL never lands, we have documentation that we can simplify the code here.
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
2 comments
// TODO(thiabaud): Entering and leaving input mode repeatedly may cause thenit: Let's use the bug number for the TODOs.
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 1 comment![]( "Open in Gerrit")
// TODO(thiabaud): Entering and leaving input mode repeatedly may cause thenit: Let's use the bug number for the TODOs.
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
- Dominik Inführ
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
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 2 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
2 comments
LGTM
static const int kMaxInputHandlingTimeMs = 3000;Thiabaud EngelbrechtHow often and long is the input mode active? With the current implementation couldn't we run into situations where we (through entering input mode constantly) never trigger a GC because we constantly recompute and increase limits in NotifyInputHandlingEnded() until we eventually fail to allocate?
Omer KatzShould be active whenever the user is interacting with Chrome. Which shouldn't be too often/too long (as seen by the results of the experiment we ran already).
I believe the check with |AllocationLimitOvershotByLargeMargin()| below should avoid this case though (please correct me if I'm wrong here).
Thiabaud EngelbrechtI think Dominik's concern was that if you exit and enter input mode repetitively, each time you exit you bump the limits. Potentially that means you may never hit the overshot by large margin case.
I initially thought that scheduling the incremental marking job is what protects us from this. However if the renderer is busy handling input, we won't get that task either. Likely there is a way to misuse input mode to continuously grow the limits.
I think this problem also exists with the current loading mode if it's misused, but I suspect the intended usage of input mode makes it more likely to accidentally be misused. E.g. if a user keeps scrolling the page up and down, will we be forever in input mode? My understanding is that the answer is yes.
This needs a mitigation, but for the sake of progress we can also add a TODO for now, land without a mitigation, and add one in a followup (before we start experimenting with it imo).
Done
I initially thought that scheduling the incremental marking job is what protects us from this. However if the renderer is busy handling input, we won't get that task either.
1- EnsureAllocationLimitAboveCurrentSize is called in Heap::StartIncrementalMarking(), at which point we already started GC (we won't change the limit until we finish it)
2- RecomputeLimitsAfterInputHandlingIfNeeded called in NotifyInputHandlingEnded() only schedules incremental GC, so this is where we can potentially continuously grow.
Maybe an easy mitigation is to call StartIncrementalMarkingIfAllocationLimitIsReached() synchronously in RecomputeLimitsAfterInputHandlingIfNeeded() instead (possibly to do as a follow-up)
Related details
- Dominik Inführ
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 1 comment![]( "Open in Gerrit")
static const int kMaxInputHandlingTimeMs = 3000;Thiabaud EngelbrechtHow often and long is the input mode active? With the current implementation couldn't we run into situations where we (through entering input mode constantly) never trigger a GC because we constantly recompute and increase limits in NotifyInputHandlingEnded() until we eventually fail to allocate?
Omer KatzShould be active whenever the user is interacting with Chrome. Which shouldn't be too often/too long (as seen by the results of the experiment we ran already).
I believe the check with |AllocationLimitOvershotByLargeMargin()| below should avoid this case though (please correct me if I'm wrong here).
Thiabaud EngelbrechtI think Dominik's concern was that if you exit and enter input mode repetitively, each time you exit you bump the limits. Potentially that means you may never hit the overshot by large margin case.
I initially thought that scheduling the incremental marking job is what protects us from this. However if the renderer is busy handling input, we won't get that task either. Likely there is a way to misuse input mode to continuously grow the limits.
I think this problem also exists with the current loading mode if it's misused, but I suspect the intended usage of input mode makes it more likely to accidentally be misused. E.g. if a user keeps scrolling the page up and down, will we be forever in input mode? My understanding is that the answer is yes.
This needs a mitigation, but for the sake of progress we can also add a TODO for now, land without a mitigation, and add one in a followup (before we start experimenting with it imo).
Etienne Pierre-DorayDone
I initially thought that scheduling the incremental marking job is what protects us from this. However if the renderer is busy handling input, we won't get that task either.
1- EnsureAllocationLimitAboveCurrentSize is called in Heap::StartIncrementalMarking(), at which point we already started GC (we won't change the limit until we finish it)
2- RecomputeLimitsAfterInputHandlingIfNeeded called in NotifyInputHandlingEnded() only schedules incremental GC, so this is where we can potentially continuously grow.
Maybe an easy mitigation is to call StartIncrementalMarkingIfAllocationLimitIsReached() synchronously in RecomputeLimitsAfterInputHandlingIfNeeded() instead (possibly to do as a follow-up)
1- EnsureAllocationLimitAboveCurrentSize is called in Heap::StartIncrementalMarking(), at which point we already started GC (we won't change the limit until we finish it)
It's also called when leaving input mode (and I think it should be called there). In that case,if the limit was exceeded only by a small margin (meaning we haven't started incremental marking yet), we will update the limits and schedule a task to start incremental marking. If that task doesn't get to run, we can do this over and over again to slowly increase the limits and never actually get a GC.
I believe the existing loading mode is also affected by this, but likely harder to trigger than with input mode.
2- RecomputeLimitsAfterInputHandlingIfNeeded called in NotifyInputHandlingEnded() only schedules incremental GC, so this is where we can potentially continuously grow.
Agreed.
Maybe an easy mitigation is to call StartIncrementalMarkingIfAllocationLimitIsReached() synchronously in RecomputeLimitsAfterInputHandlingIfNeeded() instead
We generally prefer to schedule a job and wait for it. I think we'll need another threshold where we skip the job and start incremental marking immediately (similar to the regular hard limit). However, that means we need to remember the original limit since the last GC or something like that. We'd have to think this through.
(possibly to do as a follow-up)
- Dominik Inführ
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
* which simply updates the timestamp when V8 considers the input handlingDoesn't this mean that repeatedly calling `SetIsInputHandling(true)` allows for bypassing the max time for a single input handler?
Related details
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 1 comment![]( "Open in Gerrit")
* which simply updates the timestamp when V8 considers the input handlingDoesn't this mean that repeatedly calling `SetIsInputHandling(true)` allows for bypassing the max time for a single input handler?
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
* which simply updates the timestamp when V8 considers the input handlingThiabaud EngelbrechtDoesn't this mean that repeatedly calling `SetIsInputHandling(true)` allows for bypassing the max time for a single input handler?
Yes, would you prefer for this to be a no-op?
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
RecomputeLimitsAfterInputHandlingIfNeeded();Michael, Omer and I discussed this issue again just now. For NotifyLoadingEnded, we recompute (or alternatively) reset limits here to not finalize incremental marking early. But since introducing this for NotifyLoadedingEnded, we started reseting/recomputing allocation limits on incremental marking start recently to prevent exactly this (IIRC in other scenarios) as well.
So we think that we don't need to recompute/reset limits here at all anymore. (I will update NotifyLoadingEnded() accordingly in a separate CL). So just this should be enough:
```
// .... code before ....
if (context == LeaveHeapState::kNotify) {
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already
// started) and advance marking if incremental marking is active.
job->ScheduleTask();
}
} else {
DCHECK_EQ(context, LeaveHeapState::kReachedTimeout);
// Nothing to do here because we only trigger this from a GC.
}
```
I hope this is somewhat clear. This should also solve our concern that back-to-back input phases could grow the heap without bounds. Wdyt?
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 1 comment![]( "Open in Gerrit")
RecomputeLimitsAfterInputHandlingIfNeeded();Michael, Omer and I discussed this issue again just now. For NotifyLoadingEnded, we recompute (or alternatively) reset limits here to not finalize incremental marking early. But since introducing this for NotifyLoadedingEnded, we started reseting/recomputing allocation limits on incremental marking start recently to prevent exactly this (IIRC in other scenarios) as well.
So we think that we don't need to recompute/reset limits here at all anymore. (I will update NotifyLoadingEnded() accordingly in a separate CL). So just this should be enough:
```
// .... code before ....
if (context == LeaveHeapState::kNotify) {
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already
// started) and advance marking if incremental marking is active.
job->ScheduleTask();
}
} else {
DCHECK_EQ(context, LeaveHeapState::kReachedTimeout);
// Nothing to do here because we only trigger this from a GC.
}
```I hope this is somewhat clear. This should also solve our concern that back-to-back input phases could grow the heap without bounds. Wdyt?
Done (leaving thread open if Michael wants to comment).
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Omer Katz
| Code-Review | +1 |
3 comments
DEFINE_INT(fixed_margin_for_input_handling, 0,Can we give this a real value?
I assume you won't run input mode with a fixed margin of 0.
// TODO(crbug.com/444705203): Entering and leaving input mode repeatedly mayI believe this is no longer an issue now that leaving input mode doesn't update the limits.
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 3 comments![]( "Open in Gerrit")
lgtm % comments
Thanks!
Can we give this a real value?
I assume you won't run input mode with a fixed margin of 0.
Done
// TODO(crbug.com/444705203): Entering and leaving input mode repeatedly mayI believe this is no longer an issue now that leaving input mode doesn't update the limits.
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted Code-Review+1![]( "Open in Gerrit")
- Dominik Inführ
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
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 2 comments![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Code-Review | +1 |
2 comments
Thanks a lot for the patience! LGTM modulo the comment Michael raised.
* which simply updates the timestamp when V8 considers the input handlingThiabaud EngelbrechtDoesn't this mean that repeatedly calling `SetIsInputHandling(true)` allows for bypassing the max time for a single input handler?
Michael LippautzYes, would you prefer for this to be a no-op?
Yes : )
- Etienne Pierre-Doray
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
* which simply updates the timestamp when V8 considers the input handlingThiabaud EngelbrechtDoesn't this mean that repeatedly calling `SetIsInputHandling(true)` allows for bypassing the max time for a single input handler?
Michael LippautzYes, would you prefer for this to be a no-op?
Dominik InführYes : )
+1 to this one.
This is still open, no?
DEFINE_BOOL(optimize_for_input_handling, false,Please add this to http://go/v8-garbage-collection-experiments once you add an experiment.
RecomputeLimitsAfterInputHandlingIfNeeded();Thiabaud EngelbrechtMichael, Omer and I discussed this issue again just now. For NotifyLoadingEnded, we recompute (or alternatively) reset limits here to not finalize incremental marking early. But since introducing this for NotifyLoadedingEnded, we started reseting/recomputing allocation limits on incremental marking start recently to prevent exactly this (IIRC in other scenarios) as well.
So we think that we don't need to recompute/reset limits here at all anymore. (I will update NotifyLoadingEnded() accordingly in a separate CL). So just this should be enough:
```
// .... code before ....
if (context == LeaveHeapState::kNotify) {
if (auto* job = incremental_marking()->incremental_marking_job()) {
// The task will start incremental marking (if needed not already
// started) and advance marking if incremental marking is active.
job->ScheduleTask();
}
} else {
DCHECK_EQ(context, LeaveHeapState::kReachedTimeout);
// Nothing to do here because we only trigger this from a GC.
}
```I hope this is somewhat clear. This should also solve our concern that back-to-back input phases could grow the heap without bounds. Wdyt?
Done (leaving thread open if Michael wants to comment).
I think this is done.
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 2 comments![]( "Open in Gerrit")
* which simply updates the timestamp when V8 considers the input handlingThiabaud EngelbrechtDoesn't this mean that repeatedly calling `SetIsInputHandling(true)` allows for bypassing the max time for a single input handler?
Michael LippautzYes, would you prefer for this to be a no-op?
Dominik InführYes : )
Michael Lippautz+1 to this one.
This is still open, no?
Done
Please add this to http://go/v8-garbage-collection-experiments once you add an experiment.
- Dominik Inführ
- Etienne Pierre-Doray
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
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![]( "Open in Gerrit")
- Dominik Inführ
- Omer Katz
- Thiabaud Engelbrecht
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht added 1 comment![]( "Open in Gerrit")
Michael LippautzHigh level comments:
Right now the implementation of loading and input are shared, but this is likely not what we want: one if for throughput, the other one for responsiveness.For instance, we may not want to change GC limits for input, just to temporarily waive the limit (up to a point), whereas the reset logic for limit for loading would be different (since we expect the live heap to grow, and a lot of garbage to be generated).
Also we need to handle stacking of the two hints.
So I would suggest:
- Inline the conditions like "IsLoading() and IsInputHandling()" in the calling code
- Make sure that they stack correctly
Also, is it possible to add tests? In particular, can you test the stacking? So something like input while loading, and weird overlaps like that.
Thiabaud EngelbrechtI requested a design doc for this particular project that asked for:
1. splitting of the signal (which is done)
2. finding memory limits that are reasonable for for delaying GC (as loading is effectively disabling it for most parts) (wip)
3. Figuring out returning after the "scope" was left. Stacking is triggering similar cases. (wip)I think the CL is going in the right direction but given your comments/questions here Benoit (and the fact that I want to see very similar things), I wonder if we shouldn't pause it until these questions are answered on a design doc.
Done
Related details
- Benoit Lize
- Dominik Inführ
- Etienne Pierre-Doray
- Omer Katz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thiabaud Engelbrecht (Gerrit)
Thiabaud Engelbrecht voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[heap] Add input hint
This CL adds an API to allow the embedder to tell V8 that it is
currently handling input. This allows V8 to potentially avoid doing
GCs during this window, in order to avoid jank of the input
handling.
- M include/v8-isolate.h
- M src/api/api.cc
- M src/execution/isolate.cc
- M src/execution/isolate.h
- M src/flags/flag-definitions.h
- M src/heap/heap.cc
- M src/heap/heap.h
Code-Review: +1 by Etienne Pierre-Doray, +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |