Bug 51834 - Enhancement: Add Regexp Debug Compare between JIT and Interpreter
Summary: Enhancement: Add Regexp Debug Compare between JIT and Interpreter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-03 10:51 PST by Michael Saboff
Modified: 2011-01-06 16:17 PST (History)
3 users (show)

See Also:


Attachments
Patch Adding Debug Code to Compare RegExp JIT with RegExp Interpreter. (6.61 KB, patch)
2011-01-03 15:05 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated patch addressing reviewer's concerns (7.89 KB, patch)
2011-01-06 14:53 PST, Michael Saboff
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2011-01-03 10:51:21 PST
It makes sense to add debug code to compare the results from JIT regular expressions to interpreted regular expressions.
Comment 1 Michael Saboff 2011-01-03 15:05:50 PST
Created attachment 77850 [details]
Patch Adding Debug Code to Compare RegExp JIT with RegExp Interpreter.
Comment 2 Gavin Barraclough 2011-01-06 13:21:36 PST
Comment on attachment 77850 [details]
Patch Adding Debug Code to Compare RegExp JIT with RegExp Interpreter.

Hi Michael,

Functionally the code all looks great.  I'd like to suggest three stylistic changes.

 1027 #else
 1028 #define ENABLE_YARR_JIT_DEBUG 0

The ENABLE() macro returns false for flags that aren't defined, so there should be no need to define this id YARR_JIT is not enabled.  I think we should remove this redundant code.  (Explicitly setting the flag to 0 in the block that enables YARR_JIT is good through, to broadcast that the setting exists.)

 97 #if ENABLE(YARR_JIT_DEBUG)
 98             res = JITCode;
 99         else
 100             res = ByteCode;
 101 #else
 102         return JITCode;
 103 #endif

The return on line 102 is mis-indented.  I also think this code would be slightly clearer if you copied the if statement on the line just before this block into both the if and the else.

Finally, I think it would be great to move the #if ENABLE(YARR_JIT_DEBUG) code from RegExp::match out into a new function.  It's great to land this tool, but it would also be nice to keep the actual release code in match nice and clear.

cheers,
G.
Comment 3 Michael Saboff 2011-01-06 14:53:44 PST
Created attachment 78167 [details]
Updated patch addressing reviewer's concerns

Cleaned up some other extraneous trailing and whole line white space.
Comment 4 Gavin Barraclough 2011-01-06 16:05:37 PST
Comment on attachment 78167 [details]
Updated patch addressing reviewer's concerns

Looks great Michael
Comment 5 Michael Saboff 2011-01-06 16:17:52 PST
Committed r75208: <http://trac.webkit.org/changeset/75208>