Added ability to lock strings to prevent their representation or encoding from changing.

Added ability to lock strings to prevent their representation or encoding from changing.

Use string locking to ensure consistent representation of source string during JSON parsing.

Committed: http://code.google.com/p/v8/source/detail?r=9424

[Yang, 2011-09-20 11:26:37]

On 2011/09/20 10:44:01, Lasse Reichstein wrote:

drive-by: would it make sense to lock string encoding during regexp as well? The
encoding is checked many times when using compiled regexp to ensure that the
targeted encoding did not change. Encoding is also checked when regexp execution
is interrupted, in CheckStackGuardState. All these checks could be made
obsolete.

[Rico, 2011-09-23 07:06:52]

first round of comments:

http://codereview.chromium.org/7977001/diff/5001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7977001/diff/5001/src/heap.cc#newcode2535
src/heap.cc:2535: 
Add a note about the layout of the fixed array, e.g., that we keep the number of
current elements in the first entry and then the actual strings in the
subsequent entries.

http://codereview.chromium.org/7977001/diff/5001/src/heap.cc#newcode2546
src/heap.cc:2546: if (allocation->IsFailure()) return allocation;
Why not just use our standard way of doing this using an ToObject on allocation
and then casting the result afterwards. ToObjectUnchecked just seems wrong here

http://codereview.chromium.org/7977001/diff/5001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7977001/diff/5001/src/objects-inl.h#newcode4112
src/objects-inl.h:4112: ASSERT(is_array_index_);
I think the name of this function is not descriptive anymore when this is the
case? Additionally, why no just have this method and call AddCharacterNoIndex if
is_array_index_ is false? This will make the code using this easier to read

http://codereview.chromium.org/7977001/diff/5001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7977001/diff/5001/src/objects.h#newcode6171
src/objects.h:6171: bool IsSeqAsciiString() {
Why this, we can always just call IsSequentialAscii right? Is this just to be
able to assert this?

http://codereview.chromium.org/7977001/diff/5001/src/objects.h#newcode6228
src/objects.h:6228: ASSERT(this->Object::IsSeqTwoByteString());
Same question as for ascii

http://codereview.chromium.org/7977001/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7977001/diff/5001/src/runtime.cc#newcode9054
src/runtime.cc:9054: source = FlattenGetString(source);
As discussed offline, please change comment in handles.h which states that this
will return a failure object on OOM

http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:15348: }
Could we do this again with some GC's mixed in?

http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:15349: 
Also, even though the current code just removes the first instance it fines I
think it would be nice to validate that we can add the same string several times

[Lasse Reichstein, 2011-09-23 09:54:39]

http://codereview.chromium.org/7977001/diff/5001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7977001/diff/5001/src/heap.cc#newcode2535
src/heap.cc:2535: 
Done.

http://codereview.chromium.org/7977001/diff/5001/src/heap.cc#newcode2546
src/heap.cc:2546: if (allocation->IsFailure()) return allocation;
I don't particularly like our "standard way" - the declaration of the result
must preceed the test, and the assignment is implicit in a test-call (i.e., a
test with a side-effect).
This code does the same thing and is actually readable.

Alas, I'll change it to be "compliant".

http://codereview.chromium.org/7977001/diff/5001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7977001/diff/5001/src/objects-inl.h#newcode4112
src/objects-inl.h:4112: ASSERT(is_array_index_);
I'll change this back to be an if. The *NoIndex function will be called in
almost all cases anyway, so it's better to be readable than fast here.

http://codereview.chromium.org/7977001/diff/5001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7977001/diff/5001/src/objects.h#newcode6171
src/objects.h:6171: bool IsSeqAsciiString() {
As discussed offline, this is to allow templated functions to get a quick answer
without using the generic String::IsSeqAsciiString. I'll add fast false-returns
to the other non-composite string types too.

http://codereview.chromium.org/7977001/diff/5001/src/objects.h#newcode6228
src/objects.h:6228: ASSERT(this->Object::IsSeqTwoByteString());
Same answer.

http://codereview.chromium.org/7977001/diff/5001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7977001/diff/5001/src/runtime.cc#newcode9054
src/runtime.cc:9054: source = FlattenGetString(source);
Done.

http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:15348: }
Not necessary (discussed offline). That would only check that the FixedArray is
properly treated as a GC root, which isn't the point here.

http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:15349: 
Good point! Adding test for that.

[Rico, 2011-09-26 07:25:30]

LGTM

http://codereview.chromium.org/7977001/diff/8002/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/7977001/diff/8002/src/handles.h#newcode384
src/handles.h:384: 
Could we have a small comment (as in a executive summary of the one in heap.h
:-)) One could easily wonder if this is a synchronization mechanism or a GC
prevent move of this string functionality

http://codereview.chromium.org/7977001/diff/8002/src/handles.h#newcode385
src/handles.h:385: void LockString(Handle<String> string);
Any reason not to have this as a private method in StringLock?

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h
File src/json-parser.h (right):

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h#newcode619
src/json-parser.h:619: 
add blank line

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h#newcode624
src/json-parser.h:624: 
add blank line

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h#newcode631
src/json-parser.h:631: 
add blank line

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h#newcode644
src/json-parser.h:644: 
add blank line

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h#newcode649
src/json-parser.h:649: 
add blank line

http://codereview.chromium.org/7977001/diff/8002/src/json-parser.h#newcode654
src/json-parser.h:654: 
add blank line

http://codereview.chromium.org/7977001/diff/8002/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/7977001/diff/8002/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:15428: // Unlock the first half again more.
remove more from comment?