Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(635)

Issue 10386090: Implement loop for global regexps in regexp assembler. (Closed)

Created:
8 years, 7 months ago by Yang
Modified:
8 years, 7 months ago
Reviewers:
Erik Corry
CC:
v8-dev
Visibility:
Public.

Description

Implement loop for global regexps in regexp assembler. BUG= TEST= Committed: https://code.google.com/p/v8/source/detail?r=11623

Patch Set 1 #

Total comments: 25

Patch Set 2 : fix bugs, add tests, port to x64 and arm. #

Total comments: 24
Unified diffs Side-by-side diffs Delta from patch set Stats (+701 lines, -287 lines) Patch
M src/arm/code-stubs-arm.cc View 1 2 chunks +11 lines, -6 lines 4 comments Download
M src/arm/regexp-macro-assembler-arm.h View 1 3 chunks +5 lines, -3 lines 0 comments Download
M src/arm/regexp-macro-assembler-arm.cc View 1 11 chunks +108 lines, -41 lines 14 comments Download
M src/arm/simulator-arm.h View 1 3 chunks +6 lines, -6 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 2 chunks +11 lines, -7 lines 0 comments Download
M src/ia32/regexp-macro-assembler-ia32.h View 1 3 chunks +8 lines, -3 lines 0 comments Download
M src/ia32/regexp-macro-assembler-ia32.cc View 1 11 chunks +97 lines, -35 lines 0 comments Download
M src/ia32/simulator-ia32.h View 2 chunks +4 lines, -4 lines 0 comments Download
M src/isolate.h View 1 1 chunk +1 line, -1 line 0 comments Download
M src/jsregexp.h View 1 3 chunks +17 lines, -9 lines 0 comments Download
M src/jsregexp.cc View 1 6 chunks +23 lines, -3 lines 2 comments Download
M src/regexp-macro-assembler.h View 1 3 chunks +8 lines, -1 line 0 comments Download
M src/regexp-macro-assembler.cc View 5 chunks +7 lines, -3 lines 0 comments Download
M src/regexp-macro-assembler-tracer.cc View 1 2 chunks +5 lines, -3 lines 1 comment Download
M src/runtime.cc View 1 7 chunks +116 lines, -98 lines 1 comment Download
M src/x64/code-stubs-x64.cc View 1 3 chunks +14 lines, -7 lines 0 comments Download
M src/x64/regexp-macro-assembler-x64.h View 1 4 chunks +15 lines, -7 lines 0 comments Download
M src/x64/regexp-macro-assembler-x64.cc View 1 12 chunks +100 lines, -38 lines 0 comments Download
M src/x64/simulator-x64.h View 1 2 chunks +4 lines, -4 lines 0 comments Download
M test/cctest/test-regexp.cc View 4 chunks +14 lines, -8 lines 0 comments Download
A test/mjsunit/regexp-global.js View 1 1 chunk +127 lines, -0 lines 2 comments Download

Messages

Total messages: 5 (0 generated)
Yang
PTAL. This CL is far from being completed since platform ports and tests are missing. ...
8 years, 7 months ago (2012-05-11 09:57:17 UTC) #1
Erik Corry
http://codereview.chromium.org/10386090/diff/1/src/ia32/regexp-macro-assembler-ia32.h File src/ia32/regexp-macro-assembler-ia32.h (right): http://codereview.chromium.org/10386090/diff/1/src/ia32/regexp-macro-assembler-ia32.h#newcode139 src/ia32/regexp-macro-assembler-ia32.h:139: // For the case of global regular expression, we ...
8 years, 7 months ago (2012-05-11 11:01:00 UTC) #2
Yang
Addressed comments and ported to x64 and arm. Please take another look. http://codereview.chromium.org/10386090/diff/1/src/ia32/regexp-macro-assembler-ia32.h File src/ia32/regexp-macro-assembler-ia32.h ...
8 years, 7 months ago (2012-05-16 14:58:46 UTC) #3
Erik Corry
There seems to be an issue with what happens if there is a GC and ...
8 years, 7 months ago (2012-05-22 08:32:46 UTC) #4
Yang
8 years, 7 months ago (2012-05-22 14:48:02 UTC) #5
Comments addressed and landed.

http://codereview.chromium.org/10386090/diff/3002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/10386090/diff/3002/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4850: // Argument 6: Clear the number of capture
registers for non-global capture.
On 2012/05/22 08:32:46, Erik Corry wrote:
> I don't understand this comment.  Is argument 6 a boolean argument that
> indicates whether we should clear the number of capture registers when the
> capture is non-global?  Does it have any meaning if the regexp is non-global?

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4901: // We do not expect multiple results.
On 2012/05/22 08:32:46, Erik Corry wrote:
> This comment does not clarify much.  Why do we not expect multiple results
here.

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
File src/arm/regexp-macro-assembler-arm.cc (right):

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:45: * This assembler uses the following
register assignment convention
On 2012/05/22 08:32:46, Erik Corry wrote:
> This needs updating to include r4

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:78: *  - fp[-12] start index       
(character index of start).
On 2012/05/22 08:32:46, Erik Corry wrote:
> Inconsistent capitalization, not started by you.

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:80: *  - fp[-20] success counter    (only
useful for global regexp to count matches)
On 2012/05/22 08:32:46, Erik Corry wrote:
> Does this lint?

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:745: __ mov(current_character(),
Operand('\n'), LeaveCC, eq);
On 2012/05/22 08:32:46, Erik Corry wrote:
> Having conditional instructions here after non-conditional instructions that
do
> not affect the flags - that is too tricky.  Please refactor to use
conventional
> if-then-else branching.

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:755: LoadCurrentCharacterUnchecked(-1, 1);
On 2012/05/22 08:32:46, Erik Corry wrote:
> In the global case the generated code isn't making much sense.  It's doing:
> 
> Load CurrentChar
> jmp loaded
> Load CurrentChar
> bind loaded

Done.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:894: // String might have moved: Reload
end of string from frame.
On 2012/05/22 08:32:46, Erik Corry wrote:
> If this happens will r4 be updated with the correct position?

r4 is a relative index from the end of string, therefore should not be affected
from moving the string.

http://codereview.chromium.org/10386090/diff/3002/src/arm/regexp-macro-assemb...
src/arm/regexp-macro-assembler-arm.cc:1378: offset = ip;
On 2012/05/22 08:32:46, Erik Corry wrote:
> This is setting a trap for a future programmer.  There are lots of
instructions
> that implicitly overwrite ip, so we should not use ip unless we have to, and
if
> we do it should be over 1-2 lines so you don't overlook that it is taken.

Using r4 now since it's not being used at this point.

http://codereview.chromium.org/10386090/diff/3002/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/10386090/diff/3002/src/jsregexp.cc#newcode5808
src/jsregexp.cc:5808: RegExpCompiler compiler(data->capture_count, ignore_case,
is_ascii);
On 2012/05/22 08:32:46, Erik Corry wrote:
> Optimization idea (probably for the next patch): At this point you can use
> data->tree->min_match() to see whether the regexp can do a zero length match. 
> You can pass that as a flag to the compiler and it can tell the macro
assembler
> to avoid testing for zero length matches in the repeat code on global regexps.

This is exactly what I've been looking for! I tried my luck with EatsAtLeast,
but that wouldn't work for non-capturing matches (e.g. lookahead). Thanks!

http://codereview.chromium.org/10386090/diff/3002/test/mjsunit/regexp-global.js
File test/mjsunit/regexp-global.js (right):

http://codereview.chromium.org/10386090/diff/3002/test/mjsunit/regexp-global....
test/mjsunit/regexp-global.js:50: assertEquals("2It 3was 1a 8pleasure 2to
4burn.", str);
On 2012/05/22 08:32:46, Erik Corry wrote:
> Nice test :-)

:)

Powered by Google App Engine
This is Rietveld 408576698