Issue 7230006: Inctroduce NewStrictSubstring to avoid check for SubString(str, 0, str.length...

Issue 7230006: Inctroduce NewStrictSubstring to avoid check for SubString(str, 0, str.length... (Closed)

Created:
9 years, 6 months ago by sandholm

Modified:
9 years, 6 months ago

Reviewers:
Lasse Reichstein, Vitaly Repeshko

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Inctroduce NewStrictSubstring to avoid check for SubString(str, 0, str.length). Cleanup JsonParser.

Patch Set 4 : '' #

Created: 9 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+81 lines, -46 lines)			Patch
M	src/factory.h	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	src/factory.cc	View	1 2 3	1 chunk	+10 lines, -0 lines	0 comments	Download
M	src/json.js	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	src/json-parser.h	View	1 2 3	7 chunks	+22 lines, -26 lines	0 comments	Download
M	src/runtime.cc	View	1 2 3	9 chunks	+43 lines, -19 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Vitaly Repeshko

Drive by comments. http://codereview.chromium.org/7230006/diff/6001/src/factory.cc File src/factory.cc (right): http://codereview.chromium.org/7230006/diff/6001/src/factory.cc#newcode220 src/factory.cc:220: CALL_HEAP_FUNCTION(isolate(), Assert it's a strict substring? ...

9 years, 6 months ago (2011-06-22 12:45:41 UTC) #2

Lasse Reichstein

LGTM with comments (and what Vitaly said) http://codereview.chromium.org/7230006/diff/6001/src/json-parser.h File src/json-parser.h (right): http://codereview.chromium.org/7230006/diff/6001/src/json-parser.h#newcode381 src/json-parser.h:381: if (c0_ ...

9 years, 6 months ago (2011-06-22 13:05:45 UTC) #3

sandholm

9 years, 6 months ago (2011-06-22 14:05:41 UTC) #4

http://codereview.chromium.org/7230006/diff/6001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/7230006/diff/6001/src/factory.cc#newcode220
src/factory.cc:220: CALL_HEAP_FUNCTION(isolate(),
On 2011/06/22 12:45:41, Vitaly Repeshko wrote:
> Assert it's a strict substring?

Done.

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

http://codereview.chromium.org/7230006/diff/6001/src/json-parser.h#newcode371
src/json-parser.h:371: return Handle<Smi>(Smi::FromInt((negative ? -i : i)));
On 2011/06/22 12:45:41, Vitaly Repeshko wrote:
> Contains an implicit TLS load. Pass isolate as the second argument to Handle
> ctor to avoid.

Done.
Thanks for catching that one.

http://codereview.chromium.org/7230006/diff/6001/src/json-parser.h#newcode381
src/json-parser.h:381: if (c0_ == 'e' || c0_ == 'E') {
On 2011/06/22 13:05:45, Lasse Reichstein wrote:
> Why do you think this is faster?
> The AsciiAlphaToLower should be inlined, so this just becomes:
>   if ((c0_ | 0x20) == 'e') {
You are right. I reverted this one.

http://codereview.chromium.org/7230006/diff/6001/src/json.js
File src/json.js (right):

http://codereview.chromium.org/7230006/diff/6001/src/json.js#newcode281
src/json.js:281: builder.push(value ? %QuoteJSONString(value) : '""');
On 2011/06/22 13:05:45, Lasse Reichstein wrote:
> use (value !== "") as condition.

Done.

http://codereview.chromium.org/7230006/diff/6001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7230006/diff/6001/src/runtime.cc#newcode3173
src/runtime.cc:3173: int to = offsets.at(j++);
On 2011/06/22 13:05:45, Lasse Reichstein wrote:
> Is this really faster?
> Any optimizing compiler worth its salt should be able to strength reduce this
if
> it wants to (and it shouldn't want to, since the  offsets.at(i * 2 + 1)  code
> should be implementable as a single opcode.
Sure. I'll change it back then.

Expand Messages | Collapse Messages