Create stand-alone json parser (including scanner).

Create stand-alone json parser (including scanner).

The current json parser and scanner inherits fromt he normal scanners and parsers,
which are more complicated than we need for parsing json.

The supplied scanner works directly on the string supplied and has a
fast case mode for scanning only ascii characters (it will simply
create a substring or a symbol directly from the existing string). To
allow for creating symbols from a substring I have added a
SubStringAsciiSymbolKey that creates the hash based from our string
without extracting the sub-string. In case we need to add the symbol
it simply creates the symbol directly from the characters inside the
given string.
Committed: http://code.google.com/p/v8/source/detail?r=8029

[Rico, 2011-05-18 12:08:07]

This is a first version of the stand-alone json parser. I have a bunch of
additional optimizations in mind, but I don't wan't to make this cl even
larger/complicated, so please don't review this with further optimizations in
mind.

[Lasse Reichstein, 2011-05-19 07:27:40]

Request denied.
LGTM.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc
File src/json-parser.cc (right):

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode46
src/json-parser.cc:46: is_sequential_ascii_ = true;
This is not a property that is guaranteed to be preserved over time. A string
can change from being ASCII to being two-byte due to it being used in the DOM
(because WebKit is uc16 clean internally).

I.e., you have to check again every time the code might have been interrupted.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode145
src/json-parser.cc:145: Handle<String> key = GetString(true);
How about having two functions: GetString() and GetSymbol(), that both call a
utility function with true/false (or something). Makes it easier to read, since
you don't have to know what "true" means here.
(i.e., "no magic numbers" should extend to "no magic booleans").

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode344
src/json-parser.cc:344: } else {
Ick. Why create a heap string?
Just make a buffer and do WriteToFlat of the number (you have start and end pos)
to the buffer, and parse from that using the fast atoi.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode363
src/json-parser.cc:363: ASSERT(two_byte->IsSeqTwoByteString());
Odd assert. How could that not happen?
Consider changing the return type of Factory::NewRawTwoByteString to
SeqTwoByteString.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode439
src/json-parser.cc:439: count);
If the ascii string is short (e.g., it's shorter than a cons string), you might
want to copy the ascii characters to the two-byte string instead of creating a
cons.

You might even want to not create the ASCII string first in that case.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode458
src/json-parser.cc:458: return SlowScanJsonString();
The SlowScanJsonString creates a TwoByte string. Isn't that overkill for
something containing escapes that are still ASCII (e.g., the very plausible \n,
\r, \t,\\ or \").

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode470
src/json-parser.cc:470: if (!string_val_.is_null()) return string_val_;
That does not make it a symbol if it isn't already.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode481
src/json-parser.cc:481: return  isolate()->factory()->NewSubString(
This also doesn't make it a symbol even if is_symbol is true.
If this function is not guaranteed to return a symbol when is_symbol is true,
rename it to "hint_symbol" (or something literal).

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

http://codereview.chromium.org/7039037/diff/1/src/json-parser.h#newcode46
src/json-parser.h:46: JsonParser() : isolate_(Isolate::Current()) {}
Get the isolate from the input string instead.

http://codereview.chromium.org/7039037/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7039037/diff/1/src/objects.cc#newcode5478
src/objects.cc:5478: if (Get(i) != static_cast<uint16_t>(str[i])) return false;
Potentially unnecessarily slow for deeply nested cons strings.
Consider doing a recursive version that traverses only once, and checks
subvectors agains substrings.

[Rico, 2011-05-23 18:18:12]

Please have another look

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc
File src/json-parser.cc (right):

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode46
src/json-parser.cc:46: is_sequential_ascii_ = true;
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> This is not a property that is guaranteed to be preserved over time. A string
> can change from being ASCII to being two-byte due to it being used in the DOM
> (because WebKit is uc16 clean internally).
> 
> I.e., you have to check again every time the code might have been interrupted.
As discussed offline, this is not the case right?

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode145
src/json-parser.cc:145: Handle<String> key = GetString(true);
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> How about having two functions: GetString() and GetSymbol(), that both call a
> utility function with true/false (or something). Makes it easier to read,
since
> you don't have to know what "true" means here.
> (i.e., "no magic numbers" should extend to "no magic booleans").

Done.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode344
src/json-parser.cc:344: } else {
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> Ick. Why create a heap string?
> Just make a buffer and do WriteToFlat of the number (you have start and end
pos)
> to the buffer, and parse from that using the fast atoi.
Fast atoi? this is a double.
I added a conversion method taking a uc16 vector and are using this instead

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode363
src/json-parser.cc:363: ASSERT(two_byte->IsSeqTwoByteString());
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> Odd assert. How could that not happen?
> Consider changing the return type of Factory::NewRawTwoByteString to
> SeqTwoByteString.

Done.

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode439
src/json-parser.cc:439: count);
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> If the ascii string is short (e.g., it's shorter than a cons string), you
might
> want to copy the ascii characters to the two-byte string instead of creating a
> cons.
> 
> You might even want to not create the ASCII string first in that case.
Indeed, this is one of those optimizations for the second round ;-)

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode458
src/json-parser.cc:458: return SlowScanJsonString();
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> The SlowScanJsonString creates a TwoByte string. Isn't that overkill for
> something containing escapes that are still ASCII (e.g., the very plausible
\n,
> \r, \t,\\ or \").

Yes

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode470
src/json-parser.cc:470: if (!string_val_.is_null()) return string_val_;
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> That does not make it a symbol if it isn't already.
renaming hint_symbol

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode481
src/json-parser.cc:481: return  isolate()->factory()->NewSubString(
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> This also doesn't make it a symbol even if is_symbol is true.
> If this function is not guaranteed to return a symbol when is_symbol is true,
> rename it to "hint_symbol" (or something literal).

Done.

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

http://codereview.chromium.org/7039037/diff/1/src/json-parser.h#newcode46
src/json-parser.h:46: JsonParser() : isolate_(Isolate::Current()) {}
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> Get the isolate from the input string instead.

Done.

http://codereview.chromium.org/7039037/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7039037/diff/1/src/objects.cc#newcode5478
src/objects.cc:5478: if (Get(i) != static_cast<uint16_t>(str[i])) return false;
On 2011/05/19 07:27:40, Lasse Reichstein wrote:
> Potentially unnecessarily slow for deeply nested cons strings.
> Consider doing a recursive version that traverses only once, and checks
> subvectors agains substrings.
This is the existing version, so I will leave as is for now

[Lasse Reichstein, 2011-05-24 07:28:33]

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc
File src/json-parser.cc (right):

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode46
src/json-parser.cc:46: is_sequential_ascii_ = true;
That what is not the case? That strings can change, or that the code is
interrupted?

In either case, it's not true.
The MakeExternal api function can be called from a weak callback triggered by
GC, so any time you do allocation (since this is Handle code), your string
representation could potentially change.
It probably doesn't happen, but I can't guarantee that it isn't a security
problem if it does.
I.e., you can only cache the string shape for the duration of a
AssertNoAllocation guard.
ALternatively, put a guard on the MakeExternal api function that disallows it
during GC (which is a very reasonable thing to do anyway - GC callbacks should
be restricted in what they can do, and they currently aren't).

Obviously if you allow preemption (any stack-guards), things can change at that
time too, but you don't do that here.

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode347
src/json-parser.cc:347: number_ = StringToDouble(isolate()->unicode_cache(),
You only need an ASCII buffer (we have already established that it is a number
literal, so all characters are ASCII).

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode348
src/json-parser.cc:348: *value,
res -> result.

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode354
src/json-parser.cc:354: 
Just drop the last argument (i.e., let it default to 0.0).
We know that the string isn't empty anyway.

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode478
src/json-parser.cc:478: length);
I guess that's ok ... for now.

[Rico, 2011-05-24 08:49:24]

Comments addressed

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc
File src/json-parser.cc (right):

http://codereview.chromium.org/7039037/diff/1/src/json-parser.cc#newcode46
src/json-parser.cc:46: is_sequential_ascii_ = true;
On 2011/05/24 07:28:34, Lasse Reichstein wrote:
> That what is not the case? That strings can change, or that the code is
> interrupted?
> 
> In either case, it's not true.
> The MakeExternal api function can be called from a weak callback triggered by
> GC, so any time you do allocation (since this is Handle code), your string
> representation could potentially change.
> It probably doesn't happen, but I can't guarantee that it isn't a security
> problem if it does.
> I.e., you can only cache the string shape for the duration of a
> AssertNoAllocation guard.
> ALternatively, put a guard on the MakeExternal api function that disallows it
> during GC (which is a very reasonable thing to do anyway - GC callbacks should
> be restricted in what they can do, and they currently aren't).
> 
> Obviously if you allow preemption (any stack-guards), things can change at
that
> time too, but you don't do that here.

OK, changed so that we reinitialize this variable every time we call scanjson
(this has a minor performance implication).

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode347
src/json-parser.cc:347: number_ = StringToDouble(isolate()->unicode_cache(),
On 2011/05/24 07:28:34, Lasse Reichstein wrote:
> You only need an ASCII buffer (we have already established that it is a number
> literal, so all characters are ASCII).

Done.

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode348
src/json-parser.cc:348: *value,
On 2011/05/24 07:28:34, Lasse Reichstein wrote:
> res -> result.

Done.

http://codereview.chromium.org/7039037/diff/8001/src/json-parser.cc#newcode354
src/json-parser.cc:354: 
On 2011/05/24 07:28:34, Lasse Reichstein wrote:
> Just drop the last argument (i.e., let it default to 0.0).
> We know that the string isn't empty anyway.

Done.

[Lasse Reichstein, 2011-05-24 11:28:01]

LGTM