[Webkit-unassigned] [Bug 31051] [v8] control external memory retention due to V8 objects

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=31051





--- Comment #4 from anton muhin <antonm at chromium.org>  2009-11-03 10:06:18 PDT ---
(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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.