Bug 74498

Created attachment 119203 [details] Patch

It's best to CC the JSC guys if you want this reviewed. :)

Comment on attachment 119203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119203&action=review > Source/JavaScriptCore/runtime/Options.cpp:51 > +#define CLAMP(var, min, max) var = ((var <= min) ? min : ((var >= max) ? max : var)) Perhaps it's possible to use clampTo from MathExtras.h here? It looks suspiciously like it already handles numeric_limits minimums, but I don't know if it has support for int32_t yet.

Comment on attachment 119203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119203&action=review > Source/JavaScriptCore/runtime/Options.cpp:55 > + executionCounterValueForOptimizeNextInvocation = 0; // Not configurable. That's not right. We want to be able to change this. > Source/JavaScriptCore/runtime/Options.cpp:56 > + CLAMP(executionCounterValueForOptimizeSoon, std::numeric_limits<int32_t>::min(), executionCounterValueForOptimizeNextInvocation - 1); That's probably a bit too strict. Could drop the - 1. > Source/JavaScriptCore/runtime/Options.cpp:65 > + CLAMP(desiredProfileFullnessRate, 0, desiredProfileLivenessRate); Why? These two nunbers are unrelated. > Source/JavaScriptCore/runtime/Options.cpp:66 > + CLAMP(numberOfGCMarkers, 1, 8); Not sure about this. On Mac the default is to top out at 4 markers *by default*, but we want to enable more markers. What's wrong with having, say, 24 markers? My machine has 24 logical CPUs. > Source/JavaScriptCore/runtime/Options.cpp:82 > + ASSERT((static_cast<int64_t>(executionCounterValueForOptimizeAfterLongWarmUp) << reoptimizationRetryCounterMax) >= static_cast<int64_t>(std::numeric_limits<int32_t>::min())); I think that CheckedArithmetic.h might be better than this stuff. > Source/JavaScriptCore/runtime/Options.h:68 > + m(unsigned, reoptimizationRetryCounterMax, 30) \ Why set this to 30 and then recompute it anyway?