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?
http://codereview.chromium.org/11186025/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
