Bug 72889 – Add interface for runtime option settings

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

[Andy Wingo]

JSC doesn't have many runtime knobs, instead relying on compile-time ifdefs.  I would like to add some more runtime things, for debugging, and also eventually to have a runtime --harmony flag for bug 31813.

The patch to be attached is an RFC for this sort of thing.

[Andy Wingo]

Created attachment 116096 [details]
Patch

[Andy Wingo]

Beyond the idea itself, my C++ style instincts are terrible, so feedback is appreciated :)

[Gustavo Noronha (kov)]

Comment on attachment 116096 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 116096 [details]
Patch

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

[Andy Wingo]

I'll update so it compiles on release builds, and add build support for other ports for the new files.  Feedback regarding the name would be useful, so I can add the files with the right names.

[Andy Wingo]

Created attachment 116104 [details]
Patch

[Andy Wingo]

Attached patch introduces some cruft so that accessors are always there.

To use the one flag that currently exists as an example, --dump-bytecode is only available in debug mode.  This has always been the case, under its old -d name, but now it is only present in jsc --help in debug mode.

Transcript, in debug mode:

$ ./WebKitBuild/Debug/Programs/jsc --dump-bytecode
> 1
4 m_instructions; 72 bytes at 0x1e21250; 1 parameter(s); 1 callee register(s)
[...]
> 
$ ./WebKitBuild/Debug/Programs/jsc --help
Usage: jsc [options] [files] [-- arguments]
  -e         Evaluate argument as script code
  -f         Specifies a source file (deprecated)
  -h|--help  Prints this help message
  -i         Enables interactive mode (default if no files are specified)
  -s         Installs signal handlers that exit on a crash (Unix platforms only)
  --dump-bytecode
             Dump all generated bytecode

In release mode:

$ ./WebKitBuild/Release/Programs/jsc --help
Usage: jsc [options] [files] [-- arguments]
  -e         Evaluate argument as script code
  -f         Specifies a source file (deprecated)
  -h|--help  Prints this help message
  -i         Enables interactive mode (default if no files are specified)
  -s         Installs signal handlers that exit on a crash (Unix platforms only)
$ ./WebKitBuild/Release/Programs/jsc --dump-bytecode
ERROR: This build has not been configured to allow the --dump-bytecode flag to be set at runtime.

Note the error.  In release mode, options that are not configurable compile to static methods that should inline appropriately, if I understand things right.

[Gyuyoung Kim]

Comment on attachment 116104 [details]
Patch

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

[Filip Pizlo]

Is it really necessary for this to be per-instance?  I can appreciate what the Options object might give us, but it would be unfortunate if the code became more complex in order to support per-instance run-time knobs.

Note that there is already a facility to support per-process run-time knobs.  See Heuristics.cpp.  It almost feels like this would have a better over balance between flexibility and code cleanliness if we just renamed Heuristics to Options, and started moving things like the byte code generator's dump flag into Options.

[Andy Wingo]

Sorry for my ignorance, but what do you mean by per-instance?  My patch sets global variables, basically.

I'm fine with using the Heuristics code instead.  Would you prefer that?  Would you be opposed to command-line integration, or do you think that environment variables are sufficient?  I do enjoy V8's options interface, as it is a bit more discoverable.

Andy

[Filip Pizlo]

(In reply to comment #10)
> Sorry for my ignorance, but what do you mean by per-instance?  My patch sets global variables, basically.

Ooops, it was me who was being ignorant.  I saw that Options class and assumed that you wanted to pass different instances of it around.  Now I see that it's meant to be static.

I do like the way you've structured Options, in that it should presumably allow adding new options fairly simple and provide discoverability.  But on the other hand, anything you do in Options should be done in such a way that it can be tuned from a full browser.  The environment variables are a cheap way to accomplish this.  Do you know how you'd interface this to WebCore/WebKit so that the options can be modified even for a browser?

[Early Warning System Bot]

Comment on attachment 116104 [details]
Patch

Attachment 116104 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10541008

[Andy Wingo]

(In reply to comment #11)
> I saw that Options class and assumed that you wanted to pass different instances of it around.  Now I see that it's meant to be static.

Yes, it's an abuse of classes as an organizational device.

Like I said, my C++ instincts are terrible :), but I thought that it could be a good idea to avoid accidental modification of the options / heuristics, and so hide them behind a static method. The method should inline, just in case that's important.

> Anything you do in Options should be done in such a way that it can be tuned from a full browser.  The environment variables are a cheap way to accomplish this.  Do you know how you'd interface this to WebCore/WebKit so that the options can be modified even for a browser?

We could provide the same interface, or we could provide API, or provide a little parser for e.g. JSC_HEURISTICS='likely-to-take-slow-case-threshold=4,maximum-optimization-delay=35,..'

I like your suggestion of just renaming Heuristics to Options and refactoring from there.  I'll post a patch to do that.

[Andy Wingo]

Created attachment 116193 [details]
Patch

[Andy Wingo]

Created attachment 116194 [details]
Further refactor of Options.{h,cpp}

Attached a patch to macro-ify the current heuristics.

[Andy Wingo]

Comment on attachment 116194 [details]
Further refactor of Options.{h,cpp}

Clearing review bit so the bots don't run this one.

[Andy Wingo]

Created attachment 116196 [details]
Patch

[Andy Wingo]

Fixed testRegexp.cpp, which hopefully will fix the windows build.  I'll move further patches to other bugs.

[Filip Pizlo]

Comment on attachment 116196 [details]
Patch

r=me

[Andy Wingo]

Comment on attachment 116196 [details]
Patch

cq?

[Filip Pizlo]

Comment on attachment 116196 [details]
Patch

You missed a reference to Heuristics in the Xcode project junk.  (Just do a search for Heuristics in project.pbxproj and you'll see what I mean.)

[Andy Wingo]

Created attachment 116496 [details]
Patch

[Andy Wingo]

Comment on attachment 116496 [details]
Patch

Whoops.  I think I've fixed it now.

[Andy Wingo]

Ping :)

[Andy Wingo]

Created attachment 116979 [details]
Patch

[Andy Wingo]

Updated for recent change in heuristics.

[Andy Wingo]

  Hark!  A patch revised
    packaged for your consumption
      waits expectantly

[Andy Wingo]

  A patch on a bug grows rotten
  If it feels that it has been forgotten
    You know what to do;
    I wrote you a haiku!
  R+, of limericks are you begotten?

(ok, that was pretty bad)

[Andy Wingo]

Created attachment 118760 [details]
Patch

[Andy Wingo]

Comment on attachment 118760 [details]
Patch

Filip, would you mind taking a look at this?

Thanks!

[Philippe Normand]

Created attachment 119194 [details]
Andy's updated patch

So we give it a chance on mac's EWS.

[Philippe Normand]

Created attachment 119198 [details]
Andy's updated patch

[Philippe Normand]

Created attachment 119205 [details]
Andy's updated patch

[Philippe Normand]

Created attachment 119211 [details]
Andy's updated patch

Should build on mac this time :)

[Filip Pizlo]

Comment on attachment 119211 [details]
Andy's updated patch

R=me.  Apologies for the long delay!

[WebKit Review Bot]

Comment on attachment 119211 [details]
Andy's updated patch

Clearing flags on attachment: 119211

Committed r102917: <http://trac.webkit.org/changeset/102917>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.