Adding a fast path for parsing index keys.

Adding a fast path for parsing index keys.

Reduces overhead on http://code.google.com/p/chromium/issues/detail?id=156379 from 360ms down to 255ms.

Committed: https://code.google.com/p/v8/source/detail?r=12761

[Toon Verwaest, 2012-10-18 09:20:16]

PTAL

[Yang, 2012-10-18 13:45:32]

String::ComputeArrayIndex seems to do this more cleverly. Maybe we can just
inline part of it in here. Also, maybe consider using local vars instead of
Advance, as we talked offline.

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h
File src/json-parser.h (right):

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
src/json-parser.h:305: int position = position_;
maybe something more descriptive than "position", maybe "start_pos" so that it
becomes clear that you are resetting position_.

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
src/json-parser.h:316: if (digits > 0 && c0_ == '"' && digits < 10) {
Why 10?
The logic of calculating array index (in StringHasher::AddCharacter and
String::ComputeArrayIndex) compares the intermediate result to kMaxUInt32/10 (we
should put kMaxUInt32/10 in there instead of the hardcoded literal, too). Maybe
we can just inline that logic here, so that we don't have to track the number of
digits. This way, we could also save the check for array index in the slow case
(l. 339).

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
src/json-parser.h:332: if (key.is_null()) return ReportUnexpectedCharacter();
Keeping it as
if (key.is_null() || c0_ != ':') return ReportUnexpectedCharacter();
seems alright.

[Yang, 2012-10-18 14:09:50]

On 2012/10/18 13:45:32, Yang wrote:
> String::ComputeArrayIndex seems to do this more cleverly. Maybe we can just
> inline part of it in here. Also, maybe consider using local vars instead of
> Advance, as we talked offline.
> 
> https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h
> File src/json-parser.h (right):
> 
>
https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
> src/json-parser.h:305: int position = position_;
> maybe something more descriptive than "position", maybe "start_pos" so that it
> becomes clear that you are resetting position_.
> 
>
https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
> src/json-parser.h:316: if (digits > 0 && c0_ == '"' && digits < 10) {
> Why 10?
> The logic of calculating array index (in StringHasher::AddCharacter and
> String::ComputeArrayIndex) compares the intermediate result to kMaxUInt32/10
(we
> should put kMaxUInt32/10 in there instead of the hardcoded literal, too).
Maybe
> we can just inline that logic here, so that we don't have to track the number
of
> digits. This way, we could also save the check for array index in the slow
case
> (l. 339).
> 
>
https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
> src/json-parser.h:332: if (key.is_null()) return ReportUnexpectedCharacter();
> Keeping it as
> if (key.is_null() || c0_ != ':') return ReportUnexpectedCharacter();
> seems alright.

LGTM otherwise.

[Toon Verwaest, 2012-10-18 14:15:25]

addressed comments

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h
File src/json-parser.h (right):

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
src/json-parser.h:305: int position = position_;
On 2012/10/18 13:45:32, Yang wrote:
> maybe something more descriptive than "position", maybe "start_pos" so that it
> becomes clear that you are resetting position_.

Done.

https://chromiumcodereview.appspot.com/11189039/diff/2001/src/json-parser.h#n...
src/json-parser.h:332: if (key.is_null()) return ReportUnexpectedCharacter();
On 2012/10/18 13:45:32, Yang wrote:
> Keeping it as
> if (key.is_null() || c0_ != ':') return ReportUnexpectedCharacter();
> seems alright.

Done.

[Toon Verwaest, 2012-10-18 14:30:01]

Used kMaxUInt32 as suggested.