Bug 50015 - Move regex parsing and fallback handling to runtime/RegExp.cpp
Summary: Move regex parsing and fallback handling to runtime/RegExp.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Peter Varga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-24 04:43 PST by Peter Varga
Modified: 2010-12-02 05:39 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (13.63 KB, patch)
2010-11-24 04:50 PST, Peter Varga
no flags Details | Formatted Diff | Diff
proposed patch v2 (13.72 KB, patch)
2010-11-30 06:44 PST, Peter Varga
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Varga 2010-11-24 04:43:47 PST
Since the pcre is removed from YARR the parsing of regexes by YARR is always needed either in JIT or Interpreter case.
Therefore the moving of the parsing process to runtime/RegExp.cpp is sensible and it avoids the code duplication.

On the other hand the moving of the fallback handling to the runtime/RegExp.cpp from the YARR JIT reduces
the code complexity and the code of JIT and Interpreter is seperated (JIT doesn't need to call interpret()).

These modifications make compile() and execute()/interpret() functions of YARR more simple.
Comment 1 Peter Varga 2010-11-24 04:50:34 PST
Created attachment 74747 [details]
proposed patch
Comment 2 Gavin Barraclough 2010-11-29 23:37:13 PST
Hi Peter,

A couple of cosmetic things, we should probably remove the compile method (this can now just fold back up into the constructor, since PCRE is gone, and we should probably also remove the following comment from the header:
  // FIXME: Just decompile m_regExp instead of storing this.
(not a direction I think we're headed in).

There is one way I think you could improve this patch though, see what you think.  Checks like this seem a little over complicated, particularly on a hot path:
> if (!!m_representation->m_regExpJITCode || (m_useInterpreter && m_representation->m_regExpBytecode))
(okay, it's just a couple more branches, but it could be simpler).
I'd suggest instead of adding m_useInterpreter to add an enum to track the state of the regex.  At the point of construction the RegExp always goes into one of three states - either (A) it has a parse error, or (B) if the JIT is enabled & the regex is supported it will be JIT compiled, else (C) it will be byte-compiled.  In match that if check becomes something like 'if (m_state != ParseError)'

In the longer term, it would be nice to only construct RegExp objects after successfully parsing, and have RegExp::create just return 0 on failure (with an error string as an output parameter).  Having duff RegExp objects that can't actually be run seems odd, and I think in all cases, at least within the JS languages use, errors should be reported (an thrown immediately) at construction time anyway, so there is likely no way to get a handle to a JS object that is referencing a RegExp that failed to compile.  Anyway, this would be a bigger change & is not important now.

What do you think of adding an enum to explicitly delineate the states?

cheers, G.
Comment 3 Peter Varga 2010-11-30 06:44:23 PST
Created attachment 75138 [details]
proposed patch v2

Hi Gavin,

> What do you think of adding an enum to explicitly delineate the states?
Adding an enum instead of m_useInterpreter variable is sensible and it mades the code clearer.
I updated the patch.

> we should probably remove the compile method
I didn't remove the compile method. I think it makes the code seperatable and now it makes the logic of handling
RegExp states flexible.

> we should probably also remove the following comment from the header
I removed this comment. At first the solving of the FIXME looked pointless and I didn't modify it.

Your suggestion about the modification of RegExp::create needs more investigation because It needs changes
in those classes which implement the handling of regular expressions in runtime.

Regards,
Peter
Comment 4 Gavin Barraclough 2010-12-01 10:30:12 PST
Comment on attachment 75138 [details]
proposed patch v2

> > we should probably remove the compile method
> I didn't remove the compile method. I think it makes the code seperatable and now it makes
> the logic of handling RegExp states flexible.

Fine, I'm happy with that decision Peter.  Patch looks great, r+.
Comment 5 WebKit Commit Bot 2010-12-02 02:47:38 PST
Comment on attachment 75138 [details]
proposed patch v2

Rejecting patch 75138 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
.build/Debug/testapi.build/Script-14D857B50A469C100032146C.sh


=== BUILD AGGREGATE TARGET All OF PROJECT JavaScriptCore WITH CONFIGURATION Debug ===
Check dependencies
** BUILD FAILED **


The following build commands failed:
JavaScriptCore:
	CompileC /Users/abarth/git/webkit-queue/WebKitBuild/JavaScriptCore.build/Debug/JavaScriptCore.build/Objects-normal/x86_64/RegexJIT.o /Users/abarth/git/webkit-queue/JavaScriptCore/yarr/RegexJIT.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://queues.webkit.org/results/6779006
Comment 6 Andras Becsi 2010-12-02 05:38:20 PST
Comment on attachment 75138 [details]
proposed patch v2

Landed in http://trac.webkit.org/changeset/73124, clearing flags.
Comment 7 Andras Becsi 2010-12-02 05:39:36 PST
Closing bug.