[webkit-reviews] review denied: [Bug 31051] [v8] control external memory retention due to V8 objects : [Attachment 42366] First take

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied anton muhin <antonm at chromium.org>'s
request for review:
Bug 31051: [v8] control external memory retention due to V8 objects
https://bugs.webkit.org/show_bug.cgi?id=31051

Attachment 42366: First take
https://bugs.webkit.org/attachment.cgi?id=42366&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
>+    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.