Bug 72889

Created attachment 116096 [details] Patch

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

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

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

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.

Created attachment 116104 [details] Patch

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.

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

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.

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

(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?

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

(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.

Created attachment 116193 [details] Patch

Created attachment 116194 [details] Further refactor of Options.{h,cpp} Attached a patch to macro-ify the current heuristics.

Comment on attachment 116194 [details] Further refactor of Options.{h,cpp} Clearing review bit so the bots don't run this one.

Created attachment 116196 [details] Patch

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

Comment on attachment 116196 [details] Patch r=me

Comment on attachment 116196 [details] Patch cq?

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.)

Created attachment 116496 [details] Patch

Comment on attachment 116496 [details] Patch Whoops. I think I've fixed it now.

Ping :)

Created attachment 116979 [details] Patch

Updated for recent change in heuristics.

Hark! A patch revised packaged for your consumption waits expectantly

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)

Created attachment 118760 [details] Patch

Comment on attachment 118760 [details] Patch Filip, would you mind taking a look at this? Thanks!

Created attachment 119194 [details] Andy's updated patch So we give it a chance on mac's EWS.

Created attachment 119198 [details] Andy's updated patch

Created attachment 119205 [details] Andy's updated patch

Created attachment 119211 [details] Andy's updated patch Should build on mac this time :)

Comment on attachment 119211 [details] Andy's updated patch R=me. Apologies for the long delay!

Comment on attachment 119211 [details] Andy's updated patch Clearing flags on attachment: 119211 Committed r102917: <http://trac.webkit.org/changeset/102917>

All reviewed patches have been landed. Closing bug.