Bug 67304 - Web Inspector: [Chromium] Perform a more effective JS GC
Summary: Web Inspector: [Chromium] Perform a more effective JS GC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks: 71821
  Show dependency treegraph
 
Reported: 2011-08-31 11:15 PDT by Mikhail Naganov
Modified: 2011-11-08 08:36 PST (History)
13 users (show)

See Also:


Attachments
Patch (1.82 KB, patch)
2011-08-31 11:17 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Comments addressed (1.81 KB, patch)
2011-09-01 04:39 PDT, Mikhail Naganov
pfeldman: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff
LowMemoryNotification instead of IdleNotification (870 bytes, patch)
2011-11-08 08:00 PST, Ulan Degenbaev
pfeldman: review-
pfeldman: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2011-08-31 11:15:50 PDT
We shouldn't require user to push the "GC" button several times.
Comment 1 Mikhail Naganov 2011-08-31 11:17:57 PDT
Created attachment 105799 [details]
Patch
Comment 2 Pavel Feldman 2011-08-31 22:42:06 PDT
Comment on attachment 105799 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105799&action=review

> Source/WebCore/bindings/v8/ScriptProfiler.cpp:65
> +    while (!v8::V8::IdleNotification()) {

should be { }. I'd like somebody from v8 team to bless this change before I rubber stamp it.
Comment 3 Mikhail Naganov 2011-09-01 04:39:27 PDT
Created attachment 105941 [details]
Comments addressed

Fixed braces.

I've found this code in Chromium's ChromeRenderProcessObserver::OnPurgeMemory. I've discussed it with Slava from the V8 team, and we have agreed that this approach is more effective because it actually does several rounds of GC, clears compilation caches and other stuff that will be left intact after a single MC GC round.
Comment 4 Mikhail Naganov 2011-09-01 11:39:52 PDT
Manually committed http://trac.webkit.org/changeset/94322

2011-08-31  Mikhail Naganov  <mnaganov@chromium.org>

        Web Inspector: [Chromium] Perform a more effective JS GC
        https://bugs.webkit.org/show_bug.cgi?id=67304

        Reviewed by Pavel Feldman.

        * bindings/v8/ScriptProfiler.cpp:
        (WebCore::ScriptProfiler::collectGarbage):
Comment 5 Ulan Degenbaev 2011-11-08 08:00:08 PST
Created attachment 114070 [details]
LowMemoryNotification instead of IdleNotification

The new GC fixes the LowMemoryNofication, so now there is no need to repeatedly call the IdleNotification.
Comment 6 Pavel Feldman 2011-11-08 08:01:44 PST
Comment on attachment 114070 [details]
LowMemoryNotification instead of IdleNotification

Please file a separate bug for each patch. Use webkit.org/new-inspector-bug as a template.
Comment 7 Mikhail Naganov 2011-11-08 08:26:53 PST
I'd also wait for several days before committing this change to make sure that the new V8 version sticks in Chromium, as it has been already rolled out several times due to performance regressions on bots.