Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 7477045: Tentative implementation of string slices (hidden under the flag --string-slices). (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 10 months ago by Yang
Modified:
5 years, 9 months ago
CC:
v8-dev
Visibility:
Public.

Description

Tentative implementation of string slices (hidden under the flag --string-slices). TEST=test/mjsunit/string-slices.js Committed: http://code.google.com/p/v8/source/detail?r=9027

Patch Set 1 #

Total comments: 11

Patch Set 2 : A few changes after short review by antonm. #

Total comments: 22

Patch Set 3 : Addressed some of the things pointed out by Anton. #

Total comments: 1

Patch Set 4 : Patched RegExp for string slices. #

Total comments: 9

Patch Set 5 : Implemented some changes suggested by Anton. #

Total comments: 7

Patch Set 6 : Support for RegExp. Removed methods related to truncate. #

Total comments: 7

Patch Set 7 : Included Vitaly's suggestions. #

Total comments: 29

Patch Set 8 : Implemented suggested changes. #

Total comments: 9

Patch Set 9 : Some more corrections. Offsets are set to 0 by default now. #

Total comments: 22

Patch Set 10 : Small changes here and there, mainly arch-specific code. #

Total comments: 9

Patch Set 11 : Some more suggested changes. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+1050 lines, -406 lines) Patch
M src/arm/code-stubs-arm.cc View 1 2 3 4 5 6 7 8 9 10 10 chunks +69 lines, -24 lines 1 comment Download
M src/arm/lithium-arm.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M src/arm/lithium-codegen-arm.cc View 1 2 3 4 5 6 1 chunk +44 lines, -58 lines 0 comments Download
M src/arm/regexp-macro-assembler-arm.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +21 lines, -7 lines 0 comments Download
M src/flag-definitions.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/heap.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M src/heap.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +64 lines, -13 lines 0 comments Download
M src/heap-inl.h View 3 4 5 6 7 1 chunk +4 lines, -4 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 1 2 3 4 5 6 7 8 9 10 13 chunks +92 lines, -43 lines 1 comment Download
M src/ia32/lithium-codegen-ia32.cc View 1 2 3 4 5 6 1 chunk +39 lines, -53 lines 0 comments Download
M src/ia32/lithium-ia32.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M src/ia32/regexp-macro-assembler-ia32.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +21 lines, -7 lines 0 comments Download
M src/interpreter-irregexp.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M src/jsregexp.cc View 1 2 3 4 5 6 7 8 6 chunks +15 lines, -34 lines 0 comments Download
M src/mark-compact.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 10 chunks +93 lines, -2 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 chunks +89 lines, -17 lines 0 comments Download
M src/objects-debug.cc View 1 2 3 4 5 6 7 1 chunk +25 lines, -0 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 9 chunks +84 lines, -18 lines 0 comments Download
M src/objects-visiting.h View 2 chunks +6 lines, -0 lines 0 comments Download
M src/objects-visiting.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M src/regexp-macro-assembler.cc View 1 2 3 4 5 6 2 chunks +10 lines, -6 lines 0 comments Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 2 chunks +10 lines, -15 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +72 lines, -30 lines 0 comments Download
M src/x64/lithium-codegen-x64.cc View 1 2 3 4 5 6 1 chunk +39 lines, -54 lines 0 comments Download
M src/x64/lithium-x64.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M src/x64/regexp-macro-assembler-x64.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +21 lines, -7 lines 0 comments Download
M test/cctest/test-debug.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M test/cctest/test-strings.cc View 1 2 3 4 5 2 chunks +50 lines, -2 lines 0 comments Download
A + test/mjsunit/string-slices.js View 1 2 5 chunks +65 lines, -4 lines 0 comments Download
A test/mjsunit/string-slices-regexp.js View 1 2 3 4 5 1 chunk +81 lines, -0 lines 0 comments Download
M test/mjsunit/substr.js View 1 chunk +17 lines, -0 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 25 (0 generated)
Yang
PTAL. It's probably best to start with objects.h for the definition of SlicedString and go ...
5 years, 10 months ago (2011-07-27 11:51:57 UTC) #1
antonm
+VItalyr who did a lot of string work. First round of comments. http://codereview.chromium.org/7477045/diff/1/src/heap.cc File src/heap.cc ...
5 years, 10 months ago (2011-07-27 12:16:39 UTC) #2
Yang
Made a few changes according to Anton's suggestions. http://codereview.chromium.org/7477045/diff/6002/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7477045/diff/6002/src/heap.cc#newcode2691 src/heap.cc:2691: sliced_string->set_parent(cons->first()); ...
5 years, 10 months ago (2011-07-27 12:47:24 UTC) #3
antonm
http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc#newcode4804 src/arm/code-stubs-arm.cc:4804: __ tst(result_, Operand(kStringRepresentationMask)); Do we have a spare register ...
5 years, 10 months ago (2011-07-27 14:04:49 UTC) #4
Vitaly Repeshko
There's an issue with string externalization. We can't rely on parent staying a sequential string. ...
5 years, 10 months ago (2011-07-27 15:27:24 UTC) #5
Rico
http://codereview.chromium.org/7477045/diff/6002/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7477045/diff/6002/src/objects.h#newcode524 src/objects.h:524: SLICED_SYMBOL_TYPE = kTwoByteStringTag | kSymbolTag | kSlicedStringTag, On 2011/07/27 ...
5 years, 10 months ago (2011-07-28 06:49:50 UTC) #6
Yang
I addressed a few suggestions by Anton and removed the constants for symbols. I'm still ...
5 years, 10 months ago (2011-07-28 15:43:06 UTC) #7
Yang
Added a patch for RegExp when handling sliced strings, a few tests and a set ...
5 years, 9 months ago (2011-08-04 09:19:53 UTC) #8
antonm
LGTM Might be a good idea to add verification for sliced strings. http://codereview.chromium.org/7477045/diff/20075/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc ...
5 years, 9 months ago (2011-08-04 12:18:48 UTC) #9
Yang
Made changes suggested by Anton.
5 years, 9 months ago (2011-08-04 15:09:42 UTC) #10
Vitaly Repeshko
http://codereview.chromium.org/7477045/diff/19020/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7477045/diff/19020/src/heap.cc#newcode2698 src/heap.cc:2698: ASSERT(sliced_string->parent()->IsSeqString()); I think we can have flat cons with ...
5 years, 9 months ago (2011-08-05 12:14:14 UTC) #11
Yang
I removed the family of methods related to truncate. The one I will implement in ...
5 years, 9 months ago (2011-08-11 13:23:43 UTC) #12
Vitaly Repeshko
I only looked on architecture-independent parts and on ia32 code. Since most of the new ...
5 years, 9 months ago (2011-08-12 19:00:59 UTC) #13
Yang
I did some corrections and replaced the CharCodeAt stuff with simpler code on all three ...
5 years, 9 months ago (2011-08-17 09:34:37 UTC) #14
Vitaly Repeshko
Another bunch of comments. Once again I only looked on architecture-independent parts and on ia32 ...
5 years, 9 months ago (2011-08-17 19:20:23 UTC) #15
Yang
Implemented most of the changes you (Vitaly) suggested. I also tested this with regexp=interpreted. In ...
5 years, 9 months ago (2011-08-18 12:17:32 UTC) #16
Vitaly Repeshko
LGTM for architecture-independent parts. Waiting for an updated generated code with better handling of kNotASlice ...
5 years, 9 months ago (2011-08-19 16:27:40 UTC) #17
Yang
Changed default offset to 0 instead of kNotASlice. And some minor corrections.
5 years, 9 months ago (2011-08-22 10:32:24 UTC) #18
antonm
http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#newcode4343 src/arm/code-stubs-arm.cc:4343: __ mov(r9, Operand(0)); does it belong here? maybe move ...
5 years, 9 months ago (2011-08-25 12:04:36 UTC) #19
Yang
Thanks Anton. Here's another iteration. Please have a look. http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#newcode4343 src/arm/code-stubs-arm.cc:4343: ...
5 years, 9 months ago (2011-08-25 13:40:03 UTC) #20
antonm
http://codereview.chromium.org/7477045/diff/59001/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7477045/diff/59001/src/heap.cc#newcode2651 src/heap.cc:2651: !ConsString::cast(buffer)->first()->IsSeqString())) || nit: please, indent this line for proper ...
5 years, 9 months ago (2011-08-25 16:02:45 UTC) #21
Yang
Please take another look. http://codereview.chromium.org/7477045/diff/59001/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): http://codereview.chromium.org/7477045/diff/59001/src/ia32/code-stubs-ia32.cc#newcode3415 src/ia32/code-stubs-ia32.cc:3415: __ cmp(Operand(ebx), Immediate(kConsStringTag)); On 2011/08/25 ...
5 years, 9 months ago (2011-08-26 10:44:40 UTC) #22
antonm
LGTM http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc File src/x64/code-stubs-x64.cc (right): http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc#newcode2377 src/x64/code-stubs-x64.cc:2377: __ Set(r14, 0); On 2011/08/26 10:44:40, Yang wrote: ...
5 years, 9 months ago (2011-08-26 11:29:17 UTC) #23
Vitaly Repeshko
LGTM http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc#newcode5444 src/arm/code-stubs-arm.cc:5444: __ nop(0); // Jumping as first instruction would ...
5 years, 9 months ago (2011-08-26 22:29:05 UTC) #24
Yang
5 years, 9 months ago (2011-08-29 11:44:03 UTC) #25
On 2011/08/26 22:29:05, Vitaly Repeshko wrote:
> LGTM
> 
> http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc#n...
> src/arm/code-stubs-arm.cc:5444: __ nop(0);  // Jumping as first instruction
> would crash the code generation.
> Is this a bug?
> 
> http://codereview.chromium.org/7477045/diff/62001/src/ia32/code-stubs-ia32.cc
> File src/ia32/code-stubs-ia32.cc (right):
> 
>
http://codereview.chromium.org/7477045/diff/62001/src/ia32/code-stubs-ia32.cc...
> src/ia32/code-stubs-ia32.cc:3415: STATIC_ASSERT((kConsStringTag <
> kExternalStringTag));
> Why do we need extra () here? If the assert macro requires them, let's fix the
> macro.

Those two issues are addressed by
http://code.google.com/p/v8/issues/detail?id=1644
and
http://codereview.chromium.org/7776007/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06