[ProcessPerSite] Add PrivateMemoryFootprint heuristic [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Warning: gwsq did not assign any reviewers.
Related details
- Alex Moshchuk
- Charlie Reis
- James Maclean
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Warning: gwsq did not assign any reviewers.
Related details
- Alex Moshchuk
- Charlie Reis
- Dave Tapuska
- James Maclean
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 10 comments![]( "Open in Gerrit")
Thanks. Sending a few replies now since I'm in the attention set (before I go OOO), though it looks like it's still failing some try jobs and there are still some unresolved comments.
the average size of a tab within an upper limit being reached.Charlie ReisCan you elaborate to be more precise here? I'm unclear what you mean by "average size of a tab," especially since a tab might have OOPIFs that live in a different process, and since the average size of a main frame in general can vary significantly.
Is this trying to estimate how much memory is being used by each top-level page currently in the process (including any in-process subframes in those pages)?
Friendly ping on this, because it will help me understand the goals for the algorithm you're writing.
uint64_t private_memory_footprint_bytes_ = 0u;This looks the same on both sides of the ifdef. Can it be pulled out?
// of another main frame into the memory space.I think it would be useful to provide more of an overview of what we're trying to compute here, similar to my question in the CL description. (There's a lot of ways this could be done, so it's worth laying out what we're trying to check, such as what we're assuming about the size of a main frame and what we're assuming the "memory space" is.)
// Only log the histogram once, we don't want to over-record.Once per what? It looks like context matters here, and that we're logging this only the first time that a process is *not* reused? That might be reasonable, but I'm trying to understand what the metric will mean-- it won't show if a process is later reused (e.g., if the size went down) or if the size continues to go up, but maybe that's ok? I suppose it's mostly painting a picture of where the cutoff is in practice?
(Also, does this metric need to be documented in an xml file?)
auto* host = static_cast<RenderProcessHostImpl*>(Charlie ReisPer Alex's earlier note, this looks like it's probably called from within unit tests, so I'm not sure whether this cast is safe. (If it is, we should include a comment explaining why.)
Done
bool success = memory_instrumentation::OSMetrics::FillOSMemoryDump(Dave TapuskaCurious if this is fast enough to be called on potentially several processes, on a path where a slowdown would affect start-to-commit times for navigations?
Charlie ReisOn my linux machine this is 300-400 microseconds, I haven't tried on other OSes yet, but hopefully it is similar. But I agree using a cache for a short period is likely best.
300-400 milliseconds sounds like a significant slowdown, right? This is called via MayReuseAndIsSuitableWithMainFrameThreshold and thus in a fairly critical path during navigation via GetProcessHostForSiteInstance. Is there a way to tell what impact this is having on navigation latency when it does get called, even if it gets cached for 20 seconds? (That will help us decide if we have to do something more clever or not.)
constexpr base::TimeDelta kPrimateMemoryFootprintCacheValidTime =Private?
return private_memory_footprint_bytes_;I don't see this value getting updated below when we actually compute it in the non-cached case. Did I miss that part?
}Can you help me understand why we need these changes to the platform-specific FillOSMemoryDump code?
Related details
- Alex Moshchuk
- Dave Tapuska
- James Maclean
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Reis (Gerrit)
Charlie Reis added 1 comment![]( "Open in Gerrit")
bool success = memory_instrumentation::OSMetrics::FillOSMemoryDump(Dave TapuskaCurious if this is fast enough to be called on potentially several processes, on a path where a slowdown would affect start-to-commit times for navigations?
Charlie ReisOn my linux machine this is 300-400 microseconds, I haven't tried on other OSes yet, but hopefully it is similar. But I agree using a cache for a short period is likely best.
300-400 milliseconds sounds like a significant slowdown, right? This is called via MayReuseAndIsSuitableWithMainFrameThreshold and thus in a fairly critical path during navigation via GetProcessHostForSiteInstance. Is there a way to tell what impact this is having on navigation latency when it does get called, even if it gets cached for 20 seconds? (That will help us decide if we have to do something more clever or not.)
Oh! Sorry, for misreading microseconds as milliseconds. That's much less of a concern.
Dave Tapuska (Gerrit)
Dave Tapuska added 7 comments![]( "Open in Gerrit")
the average size of a tab within an upper limit being reached.Charlie ReisCan you elaborate to be more precise here? I'm unclear what you mean by "average size of a tab," especially since a tab might have OOPIFs that live in a different process, and since the average size of a main frame in general can vary significantly.
Is this trying to estimate how much memory is being used by each top-level page currently in the process (including any in-process subframes in those pages)?
Friendly ping on this, because it will help me understand the goals for the algorithm you're writing.
Done
Bug: 348199030Dave TapuskaIs this also related to https://crbug.com/40259123, for a more general threshold that isn't specific to ProcessPerSite mode? Would this change work for that case as well, or would we need a separate threshold for when we're over the process limit?
While this lays some ground work for that. I think trying to attribute the cost of OOPIFs would need some additional work.
uint64_t private_memory_footprint_bytes_ = 0u;This looks the same on both sides of the ifdef. Can it be pulled out?
I had debated naming this as cached_private_memory_footprint_bytes_ when I originally wrote it. But I've added comments to not use it directly.
// of another main frame into the memory space.I think it would be useful to provide more of an overview of what we're trying to compute here, similar to my question in the CL description. (There's a lot of ways this could be done, so it's worth laying out what we're trying to check, such as what we're assuming about the size of a main frame and what we're assuming the "memory space" is.)
Done
// Only log the histogram once, we don't want to over-record.Once per what? It looks like context matters here, and that we're logging this only the first time that a process is *not* reused? That might be reasonable, but I'm trying to understand what the metric will mean-- it won't show if a process is later reused (e.g., if the size went down) or if the size continues to go up, but maybe that's ok? I suppose it's mostly painting a picture of where the cutoff is in practice?
(Also, does this metric need to be documented in an xml file?)
Done
constexpr base::TimeDelta kPrimateMemoryFootprintCacheValidTime =Dave TapuskaPrivate?
Done
return private_memory_footprint_bytes_;I don't see this value getting updated below when we actually compute it in the non-cached case. Did I miss that part?
No it got dropped when I rewrote the Apple and windows ports. Fixed.
Related details
- Alex Moshchuk
- Charlie Reis
- James Maclean
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
}Dave TapuskaCan you help me understand why we need these changes to the platform-specific FillOSMemoryDump code?
Removed