Reimplement a simpler version of JSON.stringify.

Reimplement a simpler version of JSON.stringify.

BUG=

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

[Yang, 2012-10-17 15:47:07]

PTAL.

I'll probably add some more test cases later on.

[Toon Verwaest, 2012-10-17 17:52:49]

Added a first round of minor and major comments.

We can still see if we want to get full-blown ping-pong between C and JavaScript
in this CL, or whether we delay it for a later CL. I think it is pretty
important though that it is added, since otherwise bailing out late in the
process may have prohibitive overhead.

Very nice speed improvement already, however!

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

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode253
src/json-stringifier.h:253: object->LookupRealNamedProperty(*key, &lookup);
Don't you want LocalLookupRealNamedProperty instead, to avoid lookup in
prototypes (and an extra indirection)?

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode259
src/json-stringifier.h:259: ASSERT(!value->IsTheHole() || lookup.IsReadOnly());
Does it matter whether the property is readonly?
You assert it is not the hole, but at the same time handle it... Missing test?

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode288
src/json-stringifier.h:288: stack_ = Handle<JSArray>::null();
Why is the stack_ set to null if we are going to throw an exception anyway?

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode447
src/json-stringifier.h:447: BasicJsonStringifier::Result
BasicJsonStringifier::SerializeObject(
We should check for interceptors (and perhaps proxies) before serializing and
jump to the JavaScript code if so; rather than abandoning the entire
intermediate work. The same for arrays.

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode453
src/json-stringifier.h:453: GetKeysInFixedArrayFor(object, LOCAL_ONLY, &threw);
GetKeysInFixedArrayFor should probably be optimized a bit better. Currently it
copies around arrays...

More importantly: if we are just looping over fast properties without any
interceptors, we can just loop over the descriptors. The descriptors are not
only stored in order of addition (enumeration order), but also directly contain
the field index or constant function. This way we can directly get access to the
value without looking it up.

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode508
src/json-stringifier.h:508:
isolate_->heap()->CreateFillerObjectAt(end_of_string, delta);
Maybe add a comment saying that this relies on word-alignment of the
string_size.

http://codereview.chromium.org/11186025/diff/1/src/json.js
File src/json.js (right):

http://codereview.chromium.org/11186025/diff/1/src/json.js#newcode94
src/json.js:94: function SerializeObject(value, replacer, stack, indent, gap) {
We should try the fast path here, rather than in JSONStringify below. If we do
the same on the C-side, we can jump back and forth as necessary, without giving
up work in between.

http://codereview.chromium.org/11186025/diff/1/src/json.js#newcode311
src/json.js:311: if (result != 0) return result;
If possible, also use IS_UNDEFINED here; like below.

http://codereview.chromium.org/11186025/diff/1/test/mjsunit/json.js
File test/mjsunit/json.js (right):

http://codereview.chromium.org/11186025/diff/1/test/mjsunit/json.js#newcode418
test/mjsunit/json.js:418: //assertEquals(1, callCount);
Is this still supposed to be commented out?

[Yang, 2012-10-18 12:27:33]

I'll do the C++/JS ping pong in a later CL. For now, I'll just bail out and
throw away everything. Please take a second look.

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

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode447
src/json-stringifier.h:447: BasicJsonStringifier::Result
BasicJsonStringifier::SerializeObject(
On 2012/10/17 17:52:50, Toon Verwaest wrote:
> We should check for interceptors (and perhaps proxies) before serializing and
> jump to the JavaScript code if so; rather than abandoning the entire
> intermediate work. The same for arrays.

I'll do this in another CL.

http://codereview.chromium.org/11186025/diff/1/src/json-stringifier.h#newcode453
src/json-stringifier.h:453: GetKeysInFixedArrayFor(object, LOCAL_ONLY, &threw);
On 2012/10/17 17:52:50, Toon Verwaest wrote:
> GetKeysInFixedArrayFor should probably be optimized a bit better. Currently it
> copies around arrays...
> 
> More importantly: if we are just looping over fast properties without any
> interceptors, we can just loop over the descriptors. The descriptors are not
> only stored in order of addition (enumeration order), but also directly
contain
> the field index or constant function. This way we can directly get access to
the
> value without looking it up.

Ditto.

http://codereview.chromium.org/11186025/diff/1/src/json.js
File src/json.js (right):

http://codereview.chromium.org/11186025/diff/1/src/json.js#newcode94
src/json.js:94: function SerializeObject(value, replacer, stack, indent, gap) {
On 2012/10/17 17:52:50, Toon Verwaest wrote:
> We should try the fast path here, rather than in JSONStringify below. If we do
> the same on the C-side, we can jump back and forth as necessary, without
giving
> up work in between.

Will do in a later CL.

http://codereview.chromium.org/11186025/diff/1/src/json.js#newcode311
src/json.js:311: if (result != 0) return result;
On 2012/10/17 17:52:50, Toon Verwaest wrote:
> If possible, also use IS_UNDEFINED here; like below.

I don't think this is necessary:
if nothing is written to the string, the return value of %BasicJSONStringify is
already undefined. Returning it has the same effect as the line

if (builder.length == 0) return;

The IS_UNDEFINED check is used to see whether %_FastAsciiArrayJoin bailed out
due to failed allocation in generated code. Undefined serves as a sentinel value
to signal using %StringBuilderConcat as fallback.

[Toon Verwaest, 2012-10-18 12:59:35]

lgtm