Bug 74498 - ensure consistent heuristics
Summary: ensure consistent heuristics
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 72938
Blocks: 72960
  Show dependency treegraph
 
Reported: 2011-12-14 04:24 PST by Andy Wingo
Modified: 2011-12-21 14:30 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.09 KB, patch)
2011-12-14 04:48 PST, Andy Wingo
fpizlo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Wingo 2011-12-14 04:24:34 PST
Patch to be attached factors consistency-checking routine out of initializeOptions, née initializeHeuristics, and clamps various values at runtime.

Depends on bug 72938.
Comment 1 Andy Wingo 2011-12-14 04:48:04 PST
Created attachment 119203 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-12-21 12:18:20 PST
It's best to CC the JSC guys if you want this reviewed. :)
Comment 3 Martin Robinson 2011-12-21 12:24:21 PST
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 4 Filip Pizlo 2011-12-21 14:30:32 PST
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?