Bug 90420 – JSC: Add ability to symbolically set and dump JSC VM options

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

It would be nice to be able to set VM options (and verify them) from the jsc command line.  This will make it easier to adjust VM heuristics for testing and tuning purposes without having to recompile JSC.

For example, to do a test run with the DFG disabled, one should be run jsc from the command line as follows:
$ jsc --useDFGJIT=false

Another example, to shows the default values for the VM settings:
$ jsc --options
JSC runtime options:
   useJIT: true
   useDFGJIT: true
   showDisassembly: false
   showDFGDisassembly: false
   maximumOptimizationCandidateInstructionCount: 10000
   maximumFunctionForCallInlineCandidateInstructionCount: 180
   maximumFunctionForConstructInlineCandidateInstructionCount: 100
   maximumInliningDepth: 5
   thresholdForJITAfterWarmUp: 100
   thresholdForJITSoon: 100
   thresholdForOptimizeAfterWarmUp: 1000
   thresholdForOptimizeAfterLongWarmUp: 5000
   thresholdForOptimizeSoon: 1000
   executionCounterIncrementForLoop: 1
   executionCounterIncrementForReturn: 15
   randomizeExecutionCountsBetweenCheckpoints: false
   maximumExecutionCountsBetweenCheckpoints: 1000
   likelyToTakeSlowCaseThreshold: 0.150000
   couldTakeSlowCaseThreshold: 0.050000
   likelyToTakeSlowCaseMinimumCount: 100
   couldTakeSlowCaseMinimumCount: 10
   osrExitProminenceForFrequentExitSite: 0.300000
   osrExitCountForReoptimization: 100
   osrExitCountForReoptimizationFromLoop: 5
   reoptimizationRetryCounterMax: 18
   reoptimizationRetryCounterStep: 1
   minimumOptimizationDelay: 1
   maximumOptimizationDelay: 5
   desiredProfileLivenessRate: 0.750000
   desiredProfileFullnessRate: 0.350000
   doubleVoteRatioForDoubleFormat: 2.000000
   minimumNumberOfScansBetweenRebalance: 100
   gcMarkStackSegmentSize: 4096
   numberOfGCMarkers: 7
   opaqueRootMergeThreshold: 1000
   forceWeakRandomSeed: false
   forcedWeakRandomSeed: 0

[Mark Lam]

Created attachment 150516 [details]
Fix.

In addition to refactoring the options system, I also added a "useDFGJIT" option (true buy default) and removed the "thresholdForOptimizeNextInvocation" option.  The latter was already not being used anywhere previously.  Also added comments about how the options work at the top of runtime/Options.h.

[WebKit Review Bot]

Attachment 150516 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/Options.h:162:  int32_tVal is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

FYI, the "int32_tVal" field was named that way so that I can do some macro magic for concatinating the "int32_t" type with "Val" to form the field name.  I'm requesting that we let this instance be an exception to the style rule.  Thanks.

[Gyuyoung Kim]

Comment on attachment 150516 [details]
Fix.

Attachment 150516 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13126331

[Build Bot]

Comment on attachment 150516 [details]
Fix.

Attachment 150516 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13125337

[Mark Lam]

Created attachment 150518 [details]
Rev 1: fixed OptionID operator++ complaint.

[WebKit Review Bot]

Attachment 150518 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/Options.h:162:  int32_tVal is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 150518 [details]
Rev 1: fixed OptionID operator++ complaint.

Attachment 150518 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13137229

[Mark Lam]

Created attachment 150523 [details]
rev 2: Need to include <wtf/StringExtras.h> to get the windows build to be happy for strcasecmp.

[WebKit Review Bot]

Attachment 150523 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/runtime/Options.h:162:  int32_tVal is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #3)
> FYI, the "int32_tVal" field was named that way so that I can do some macro magic for concatinating the "int32_t" type with "Val" to form the field name.  I'm requesting that we let this instance be an exception to the style rule.  Thanks.

Yeah, in general we make lots of exceptions like this.  I think this is sensible.

[Filip Pizlo]

(In reply to comment #9)
> Created an attachment (id=150523) [details]
> rev 2: Need to include <wtf/StringExtras.h> to get the windows build to be happy for strcasecmp.

Is this meant to be r? cq?

[Mark Lam]

Waiting for the windows build bot to confirm my build fix.  Will r? cq? after.  Thanks.

[Filip Pizlo]

Comment on attachment 150523 [details]
rev 2: Need to include <wtf/StringExtras.h> to get the windows build to be happy for strcasecmp.

View in context: https://bugs.webkit.org/attachment.cgi?id=150523&action=review

I'm happy with this patch!  Please consider the suggestions though, and mark as r? cq? when you're ready.

> Source/JavaScriptCore/jsc.cpp:618
> +    fprintf(stderr, "  --options                  Dumps all JSC VM options and exits\n");

Typo: "and exit"

> Source/JavaScriptCore/jsc.cpp:619
> +    fprintf(stderr, "  --dumpOptions              Dumps all JSC VM options before continuing\n");

Is this option needed?

> Source/JavaScriptCore/runtime/Options.h:34
> +// How JSC VM options work?

Typo: "How do JSC VM options work?"

> Source/JavaScriptCore/runtime/Options.h:153
> +        TYPE_bool,
> +        TYPE_unsigned,
> +        TYPE_double,
> +        TYPE_int32_t

I realize that there's no good way of making these follow naming conventions due to the need to macro-concatenate things, but it would be better to at least follow the rule that all-caps is reserved for the names of macros.  As such, I think it would be better to use "Type_bool" than "TYPE_bool".

>> Source/JavaScriptCore/runtime/Options.h:162
>> +            bool boolVal;
>> +            unsigned unsignedVal;
>> +            double doubleVal;
>> +            int32_t int32_tVal;
> 
> int32_tVal is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

One way to make this nicer would be to make these Val_bool rather than boolVal.  Then, the naming here would match the naming of types.  Would that work with the macro magic?

[Build Bot]

Comment on attachment 150523 [details]
rev 2: Need to include <wtf/StringExtras.h> to get the windows build to be happy for strcasecmp.

Attachment 150523 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13133281

[Mark Lam]

Created attachment 150544 [details]
rev 3: Used typedef trick to eliminate the '_' altogether + addressed Filip's comments.

The windows build seemed to refuse to build because the style checker had complained about the '_' in the var names.  I've applied Gavin's typedef trick to alias int32_t as int32 inside the Option class so as to workaround this issue.

Regarding Filip's comments:

>> Source/JavaScriptCore/jsc.cpp:618
>>+    fprintf(stderr, "  --options                  Dumps all JSC VM options and exits\n");
>
>Typo: "and exit"

The comment says "Dumps ... and exits".  This is consistent in being 3rd person singular, which many of the pre-existing comments were also.  A quick survey of 3rd person singular forms of "exit" on the net seems to agree that "exits" is the proper form.  So, I'm leaving it as is.

>>Source/JavaScriptCore/jsc.cpp:619
>>+    fprintf(stderr, "  --dumpOptions              Dumps all JSC VM options before continuing\n");
>
>Is this option needed?

Addressed in previous comment.  It's useful when we want to invoke a test with some options, and wanting the options documented in the output of the test in one swoop.  Plus, it's just a  few lines of code.  I would prefer to keep it.

The other issues are all now fixed.  Will set flags to r? cq? when the build bot are happy.

[Mark Lam]

Oops, I never did publish my original response about "--dumpOptions".  Hence, there is no "previous comment" unlike what I said.  Anyway, the rational is that these command line flags are meant to make it convenient to run the VM with difference heuristic values for testing and tuning.  Adding the "--dumpOptions" flag will allow us to keep the options dump together with the test output dump from the same run for easier tracking.

[Filip Pizlo]

(In reply to comment #17)
> Oops, I never did publish my original response about "--dumpOptions".  Hence, there is no "previous comment" unlike what I said.  Anyway, the rational is that these command line flags are meant to make it convenient to run the VM with difference heuristic values for testing and tuning.  Adding the "--dumpOptions" flag will allow us to keep the options dump together with the test output dump from the same run for easier tracking.

Ah, that does sound useful.

[Gustavo Noronha (kov)]

Comment on attachment 150544 [details]
rev 3: Used typedef trick to eliminate the '_' altogether + addressed Filip's comments.

Attachment 150544 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13126395

[Build Bot]

Comment on attachment 150544 [details]
rev 3: Used typedef trick to eliminate the '_' altogether + addressed Filip's comments.

Attachment 150544 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13137294

[Mark Lam]

Created attachment 150556 [details]
rev 4: one more change to defer option dumping and exiting till after all options have been parsed.

This last change (rev 4) is to defer the dumping of options and exiting till after other options have been parsed.  Otherwise, the options being dumped may not reflect all the options that being set (from the command line args) for the current run (which what a user would expect).

The windows build still seems to be failing but the cause is unknown.  I'm sub

[Mark Lam]

Created attachment 150559 [details]
rev 5: Added missing #include "JSExportMacros.h" in Options.h.

Previous build reports were complaining of unresolved exported values.  The missing #include of JSExportMacros.h would explain this.  This is now fixed in rev5 of the patch.  Submitting for EWS test.   Will submit for r? cq? if build bots are happy.

[Build Bot]

Comment on attachment 150559 [details]
rev 5: Added missing #include "JSExportMacros.h" in Options.h.

Attachment 150559 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13130407

[Andy Wingo]

See also bug 72960.

[Gustavo Noronha (kov)]

Comment on attachment 150559 [details]
rev 5: Added missing #include "JSExportMacros.h" in Options.h.

Attachment 150559 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13127435

[Mark Lam]

Created attachment 150630 [details]
rev 6: + mods to vcproj's .def to satisfy Windows. Also removed an unrelated file from previous patch. 

rev 6: Will wait for build bots to green light before r? cq?.

[Mark Lam]

Comment on attachment 150630 [details]
rev 6: + mods to vcproj's .def to satisfy Windows. Also removed an unrelated file from previous patch. 

The bots are green.  Ready to review and commit.

[WebKit Review Bot]

Comment on attachment 150630 [details]
rev 6: + mods to vcproj's .def to satisfy Windows. Also removed an unrelated file from previous patch. 

Clearing flags on attachment: 150630

Committed r121798: <http://trac.webkit.org/changeset/121798>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.