[webkit-reviews] review denied: [Bug 74498] ensure consistent heuristics : [Attachment 119203] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Andy Wingo <wingo at igalia.com>'s
request for review:
Bug 74498: ensure consistent heuristics
https://bugs.webkit.org/show_bug.cgi?id=74498

Attachment 119203: Patch
https://bugs.webkit.org/attachment.cgi?id=119203&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
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?