Created attachment 42366 [details] First take

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.

Created attachment 42392 [details] Addressing style issues

(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 on attachment 42392 [details] Addressing style issues Great! Thanks for the changes. :)

(In reply to comment #5) > (From update of attachment 42392 [details]) > Great! Thanks for the changes. :) Thanks a lot for review, Adam!

Comment on attachment 42392 [details] Addressing style issues Clearing flags on attachment: 42392 Committed r50562: <http://trac.webkit.org/changeset/50562>

All reviewed patches have been landed. Closing bug.

Created attachment 42880 [details] Trying to reapply Begging to reapply already LGTMed patch

Guys, I just want to reapply rolledback change which was already reviewed.

Whoever did the revert really should have recorded it in this bug. :( I'll let Adam have first-crack at re-reviewing this.

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 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.

(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.

(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 on attachment 42880 [details] Trying to reapply Clearing flags on attachment: 42880 Committed r50752: <http://trac.webkit.org/changeset/50752>

All reviewed patches have been landed. Closing bug.