New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 793789 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Hotlist-MemoryInfra



Sign in to add a comment

Propagate the number of V8 native contexts and detached contexts to UKM memory metric

Project Member Reported by u...@chromium.org, Dec 11 2017

Issue description

V8 allocates a native context per iframe. When an iframe navigates to another URL or is removed, its native context becomes detached.

Detached contexts must be reclaimed on subsequent garbage collections.

By having the number of native contexts and detached contexts in UKM we will get insights about potential memory leaks:
1) High number of native contexts indicates a memory leak in JS application.
2) High number of detached contexts indicates a memory leak in Chrome/V8.

Knowing the distribution of native contexts across the real webpages also improves our understanding of memory usage in the wild.

primiano@, we discussed this during last BlinkOn.

Should I go ahead and implement this, or did something change in the meantime that would make it less useful?
 
Cc: lalitm@chromium.org
Just to understand, are we talking about exposing some #contextes, or their memory size?
I'd say:
- definitely do the work to expose that as a column in memory-infra. The code that dumps v8 stats is in src/gin/v8_isolate_memory_dump_provider.cc . 
Once that is done there are two ways:
A: manually plumbing that all the way through chrome_metrics_emitter.cc (similarly to https://chromium-review.googlesource.com/c/808445/)
B: wait for lalitm@ to finish his work (eta this Q) to get the full memory graph available to chrome_metrics_emitter.cc. That would reduce the amount of changes to few lines in chrome_metrics_emitter.cc, and avoid all the rest of the boilerplate that you see in the CL linked in A.

In both cases, it seems that any addition to UKM requires a re-iteration on the UKM collection doc (e.g. go/rdjvy is what I did last time). It's usually never a problem and a matter of few mins, but takes at least a week until gets through the queue of the people who can stamp it. So I'd start doing that in parallel to avoid bubbles in the pipeline.

Comment 2 by u...@chromium.org, Dec 11 2017

Thanks for the detailed answer, primiano@!

I'll plumb src/gin/v8_isolate_memory_dump_provider.cc and rely on lalitm@ work to pick up the new data in chrome_metrics_emitter.cc. ETA this Q works well for us.

lalitm@, is there a bug I could subscribe to for updates?

> Just to understand, are we talking about exposing some #contextes, or their memory size?
It the number of contexts. We do not know memory size per context (due to huge runtime overhead of tracking it). Also there is object sharing between contexts.

> In both cases, it seems that any addition to UKM requires a re-iteration on the UKM collection doc
I'll create a copy of the document, add new items, and send out for review as "December edition".


Comment 3 by lalitm@chromium.org, Dec 11 2017

crbug/728199 is the one I will be using for all metrics related memory graph work!
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/298f0cd4389654e36a878410fe5ff92da255e32a

commit 298f0cd4389654e36a878410fe5ff92da255e32a
Author: Ulan Degenbaev <ulan@chromium.org>
Date: Mon Dec 11 18:14:31 2017

[heap] Provide the number of native and detached context via Heap API.

This adds two fields to the HeapStatistics struct:
- number_of_native_contexts,
- number_of_detached_contexts.

Bug: chromium:793789
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: If6942a97fd22a9e70781eed2aa286aba4c0e7f70
Reviewed-on: https://chromium-review.googlesource.com/819730
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50007}
[modify] https://crrev.com/298f0cd4389654e36a878410fe5ff92da255e32a/include/v8.h
[modify] https://crrev.com/298f0cd4389654e36a878410fe5ff92da255e32a/src/api.cc
[modify] https://crrev.com/298f0cd4389654e36a878410fe5ff92da255e32a/src/heap/heap.cc
[modify] https://crrev.com/298f0cd4389654e36a878410fe5ff92da255e32a/src/heap/heap.h
[modify] https://crrev.com/298f0cd4389654e36a878410fe5ff92da255e32a/test/cctest/test-api.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bfe58719079deaff42e43116fdc9735881837cc2

commit bfe58719079deaff42e43116fdc9735881837cc2
Author: Ulan Degenbaev <ulan@chromium.org>
Date: Fri Dec 15 19:40:54 2017

[tracing] Dump context counts in v8 memory dump provider.

This patch adds the following dumps in v8 memory dump provider:
- v8/isolate/contexts/detached_context
- v8/isolate/contexts/native_context
Both dumps contain an "object_count" scalar value.

The value of detached_context is the number of contexts that were
detached (due to iframe navigation or removal) and not yet garbage
collected. This number being non-zero indicates potential memory leak.

The value of native_context is the number of the top-level contexts
created for the main window and iframes. Increase of this number
over time indicates a memory leak.

Bug: 793789
Change-Id: I4753454747c398fac4fc153fcccb1288d28f2a2d
Reviewed-on: https://chromium-review.googlesource.com/822336
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524438}
[modify] https://crrev.com/bfe58719079deaff42e43116fdc9735881837cc2/base/trace_event/memory_infra_background_whitelist.cc
[modify] https://crrev.com/bfe58719079deaff42e43116fdc9735881837cc2/gin/v8_isolate_memory_dump_provider.cc
[modify] https://crrev.com/bfe58719079deaff42e43116fdc9735881837cc2/gin/v8_isolate_memory_dump_provider_unittest.cc

Sign in to add a comment