Bug 31051

Summary: [v8] control external memory retention due to V8 objects
Product: WebKit Reporter: anton muhin <antonm>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, hclam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
First take
abarth: review-
Addressing style issues
none
Trying to reapply none

Description anton muhin 2009-11-03 04:33:17 PST
There are cases when small V8 handlers (too small to force V8 GC) retain a lot of DOM objects and thus Chromium could run out of memory even though those objects are semantic garbage.

This patch adds some checks and if overall memory usage is high, attempts to release the memory.
Comment 1 anton muhin 2009-11-03 04:48:10 PST
Created attachment 42366 [details]
First take
Comment 2 Adam Barth 2009-11-03 08:23:06 PST
Comment on attachment 42366 [details]
First take

>+    V8GCController::checkMemoryUsage();

I presume you've benchmarked this and seen that this isn't slowing us down too much.

>+        static const int LOW_USAGE_MB = 256;  // If memory usage is below this threshold, do not bother forcing GC.
>+        static const int HIGH_USAGE_MB = 1024;  // If memory usage is above this threshold, force GC more aggresively.
>+        static const int HIGH_USAGE_DELTA_MB = 128;  // Delta of memory usage growth (vs. last s_workingSetEstimateMB) to force GC when memory usage is high.

It's easier if you move these values into the cpp file since they're private anyway.  What you've done where will confuse compilers on some platforms because they're not sure which compilation out to provide storage for these values should any code take their address.

Also, the style for these variables is incorrect.  They ought to be in camelCase.

> + s_workingSetEstimateMB

I don't think this is supposed to have an "s_", I'm unclear on the style for these.  I think we're inconsistent.

>+int GetMemoryUsageInMB()

Please either pre-pend "V8GCController::" or declare this in an anonymous namespace.  Also, per the style guide, this should start with a lower-case "g".

>+{
>+    return ChromiumBridge::memoryUsageMB();
>+}

What's the point of this function?  Why not just call ChromiumBridge::memoryUsageMB?  The abstract does not appear to be buying us much.
Comment 3 anton muhin 2009-11-03 10:01:55 PST
Created attachment 42392 [details]
Addressing style issues
Comment 4 anton muhin 2009-11-03 10:06:18 PST
(In reply to comment #2)
> (From update of attachment 42366 [details])
> >+    V8GCController::checkMemoryUsage();
> 
> I presume you've benchmarked this and seen that this isn't slowing us down too
> much.

I did for some DOM operations.  As was discussed offline, would run through some more benchmarks.

> 
> >+        static const int LOW_USAGE_MB = 256;  // If memory usage is below this threshold, do not bother forcing GC.
> >+        static const int HIGH_USAGE_MB = 1024;  // If memory usage is above this threshold, force GC more aggresively.
> >+        static const int HIGH_USAGE_DELTA_MB = 128;  // Delta of memory usage growth (vs. last s_workingSetEstimateMB) to force GC when memory usage is high.
> 
> It's easier if you move these values into the cpp file since they're private
> anyway.  What you've done where will confuse compilers on some platforms
> because they're not sure which compilation out to provide storage for these
> values should any code take their address.
> 

Oh irony, that was that way before, but one of early reviewers asked to lift into the class decl, happily moving back into the method.

> Also, the style for these variables is incorrect.  They ought to be in
> camelCase.

Fixed.

> > + s_workingSetEstimateMB
> 
> I don't think this is supposed to have an "s_", I'm unclear on the style for
> these.  I think we're inconsistent.

As I don't like prefices, fixed and I feel happy :)

> 
> >+int GetMemoryUsageInMB()
> 
> Please either pre-pend "V8GCController::" or declare this in an anonymous
> namespace.  Also, per the style guide, this should start with a lower-case "g".

Move into anonymous namespace.

> >+{
> >+    return ChromiumBridge::memoryUsageMB();
> >+}
> 
> What's the point of this function?  Why not just call
> ChromiumBridge::memoryUsageMB?  The abstract does not appear to be buying us
> much.

It's up to you (I'd remove it if you like).  The idea is there might be different ways to estimate amount of used memory.  E.g. one plan was to use memory allocated for WebCore objects only.  Or we might use allocator stats, etc.  So I thought that abstracting it into a separate function might be worth it, but I don't feel to strong for this solution.

And thanks a lot for your comments!
Comment 5 Adam Barth 2009-11-03 22:54:38 PST
Comment on attachment 42392 [details]
Addressing style issues

Great!  Thanks for the changes.  :)
Comment 6 anton muhin 2009-11-05 07:22:43 PST
(In reply to comment #5)
> (From update of attachment 42392 [details])
> Great!  Thanks for the changes.  :)

Thanks a lot for review, Adam!
Comment 7 WebKit Commit Bot 2009-11-05 07:36:21 PST
Comment on attachment 42392 [details]
Addressing style issues

Clearing flags on attachment: 42392

Committed r50562: <http://trac.webkit.org/changeset/50562>
Comment 8 WebKit Commit Bot 2009-11-05 07:36:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 anton muhin 2009-11-10 11:22:59 PST
Created attachment 42880 [details]
Trying to reapply

Begging to reapply already LGTMed patch
Comment 10 anton muhin 2009-11-10 11:24:29 PST
Guys,

I just want to reapply rolledback change which was already reviewed.
Comment 11 Eric Seidel (no email) 2009-11-10 11:25:53 PST
Whoever did the revert really should have recorded it in this bug. :(  I'll let Adam have first-crack at re-reviewing this.
Comment 12 Hin-Chung Lam 2009-11-10 11:37:46 PST
Sorry I didn't post a message after the revert.

The failure in Chromium is that this code will cause delay loading of psapi.dll in XP. This is now fixed in Chromium http://codereview.chromium.org/376023. So this change should be safe to be merge into Chromium now.
Comment 13 Adam Barth 2009-11-10 11:38:10 PST
Comment on attachment 42880 [details]
Trying to reapply

(In reply to comment #10)
> I just want to reapply rolledback change which was already reviewed.

As a committer, you can do that without another review.  Just make sure you understand why the patch was rolled back.
Comment 14 Hin-Chung Lam 2009-11-10 11:38:51 PST
(In reply to comment #12)
> Sorry I didn't post a message after the revert.
> 
> The failure in Chromium is that this code will cause delay loading of psapi.dll
> in XP. This is now fixed in Chromium http://codereview.chromium.org/376023. So
> this change should be safe to be merge into Chromium now.

And delay loading a DLL in Chromium's renderer was failing because of the sandbox. Now delay loading is disabled so it's always loaded.
Comment 15 anton muhin 2009-11-10 11:42:00 PST
(In reply to comment #14)
> (In reply to comment #12)
> > Sorry I didn't post a message after the revert.
> > 
> > The failure in Chromium is that this code will cause delay loading of psapi.dll
> > in XP. This is now fixed in Chromium http://codereview.chromium.org/376023. So
> > this change should be safe to be merge into Chromium now.
> 
> And delay loading a DLL in Chromium's renderer was failing because of the
> sandbox. Now delay loading is disabled so it's always loaded.

Thanks to everyone.

to Adam: will remember that I don't need review+ --- thanks a lot for letting me know.
Comment 16 WebKit Commit Bot 2009-11-10 12:00:41 PST
Comment on attachment 42880 [details]
Trying to reapply

Clearing flags on attachment: 42880

Committed r50752: <http://trac.webkit.org/changeset/50752>
Comment 17 WebKit Commit Bot 2009-11-10 12:00:46 PST
All reviewed patches have been landed.  Closing bug.