Bug 90420 - JSC: Add ability to symbolically set and dump JSC VM options
Summary: JSC: Add ability to symbolically set and dump JSC VM options
Status: RESOLVED FIXED
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:
Blocks:
 
Reported: 2012-07-02 18:20 PDT by Mark Lam
Modified: 2012-07-03 12:19 PDT (History)
6 users (show)

See Also:


Attachments
Fix. (47.94 KB, patch)
2012-07-02 18:58 PDT, Mark Lam
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Rev 1: fixed OptionID operator++ complaint. (47.93 KB, patch)
2012-07-02 19:21 PDT, Mark Lam
mark.lam: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
rev 2: Need to include <wtf/StringExtras.h> to get the windows build to be happy for strcasecmp. (deleted)
2012-07-02 20:41 PDT, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
rev 3: Used typedef trick to eliminate the '_' altogether + addressed Filip's comments. (48.82 KB, patch)
2012-07-02 23:24 PDT, Mark Lam
gustavo: commit-queue-
Details | Formatted Diff | Diff
rev 4: one more change to defer option dumping and exiting till after all options have been parsed. (49.42 KB, patch)
2012-07-03 01:23 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
rev 5: Added missing #include "JSExportMacros.h" in Options.h. (49.47 KB, patch)
2012-07-03 01:42 PDT, Mark Lam
buildbot: commit-queue-
Details | Formatted Diff | Diff
rev 6: + mods to vcproj's .def to satisfy Windows. Also removed an unrelated file from previous patch. (50.11 KB, patch)
2012-07-03 09:42 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2012-07-02 18:20:33 PDT
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
Comment 1 Mark Lam 2012-07-02 18:58:48 PDT
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.
Comment 2 WebKit Review Bot 2012-07-02 19:02:51 PDT
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.
Comment 3 Mark Lam 2012-07-02 19:06:30 PDT
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.
Comment 4 Gyuyoung Kim 2012-07-02 19:09:30 PDT
Comment on attachment 150516 [details]
Fix.

Attachment 150516 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13126331
Comment 5 Build Bot 2012-07-02 19:17:54 PDT
Comment on attachment 150516 [details]
Fix.

Attachment 150516 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13125337
Comment 6 Mark Lam 2012-07-02 19:21:43 PDT
Created attachment 150518 [details]
Rev 1: fixed OptionID operator++ complaint.
Comment 7 WebKit Review Bot 2012-07-02 19:24:46 PDT
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.
Comment 8 Build Bot 2012-07-02 19:49:49 PDT
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
Comment 9 Mark Lam 2012-07-02 20:41:01 PDT
Created attachment 150523 [details]
rev 2: Need to include <wtf/StringExtras.h> to get the windows build to be happy for strcasecmp.
Comment 10 WebKit Review Bot 2012-07-02 20:44:21 PDT
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.
Comment 11 Filip Pizlo 2012-07-02 21:03:54 PDT
(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.
Comment 12 Filip Pizlo 2012-07-02 21:04:15 PDT
(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?
Comment 13 Mark Lam 2012-07-02 21:05:50 PDT
Waiting for the windows build bot to confirm my build fix.  Will r? cq? after.  Thanks.
Comment 14 Filip Pizlo 2012-07-02 21:09:19 PDT
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?
Comment 15 Build Bot 2012-07-02 21:14:34 PDT
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
Comment 16 Mark Lam 2012-07-02 23:24:33 PDT
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.
Comment 17 Mark Lam 2012-07-02 23:29:29 PDT
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.
Comment 18 Filip Pizlo 2012-07-02 23:33:29 PDT
(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.
Comment 19 Gustavo Noronha (kov) 2012-07-03 01:09:06 PDT
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
Comment 20 Build Bot 2012-07-03 01:23:01 PDT
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
Comment 21 Mark Lam 2012-07-03 01:23:13 PDT
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
Comment 22 Mark Lam 2012-07-03 01:42:23 PDT
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.
Comment 23 Build Bot 2012-07-03 02:38:11 PDT
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
Comment 24 Andy Wingo 2012-07-03 02:57:11 PDT
See also bug 72960.
Comment 25 Gustavo Noronha (kov) 2012-07-03 02:57:36 PDT
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
Comment 26 Mark Lam 2012-07-03 09:42:31 PDT
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?.
Comment 27 Mark Lam 2012-07-03 11:01:35 PDT
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.
Comment 28 WebKit Review Bot 2012-07-03 12:19:40 PDT
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>
Comment 29 WebKit Review Bot 2012-07-03 12:19:45 PDT
All reviewed patches have been landed.  Closing bug.