JSON stringify collects substrings in one builder array rather than using reg...

JSON stringify collects substrings in one builder array rather than using regular
string cons.

[Erik Corry, 2010-12-05 15:09:06]

STV!

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

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode213
src/json.js:213: if (builder.pop() != ",") {
This seems a bit indirect.  I think it would be clearer to just say if (len ==
0).  Is this version faster?

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode229
src/json.js:229: builder.push(%QuoteJSONString(p), ":");
If you didn't already try it I think it would be interesting to experiment with
having QuoteJSONString not add the quotes.  We should be faster at adding the
quotes now and it would open up the possibility of just returning the string
unchanged which should be the common case.  We can save that for another change
though.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode241
src/json.js:241: if (builder.pop() != ",") {
Again, I find this convoluted.  A boolean variable that gets set if the for loop
is non-empty would be clearer.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode258
src/json.js:258: builder.push(($isFinite(value) ? $String(value) : "null"));
I wonder how $String compares speedwise to ("" + value).

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270
src/json.js:270: } else { // Regular non-wrapped object
There should be 2 spaces before //

[Lasse Reichstein, 2010-12-06 08:48:45]

LVGTM too.

Maybe worth applying to the non-basic Stringify too.

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

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode210
src/json.js:210: builder.push(",");
An idiom often used in "picket fence loops" like this is 
to start with 
 if (i > 0) builder.push(",");
The overhead of the BasicJSONSerialize call will overshadow the overhead of an
extra test inside the loop, so I doubt it will be visible in the performance.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode258
src/json.js:258: builder.push(($isFinite(value) ? $String(value) : "null"));
Use %_NumberToString instead of the generic $String. You know it's a number. (Or
use NonStringToString, but it will just test for being a number and call
%_NumberToString)

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode264
src/json.js:264: value = $Number(value);
use %_ValueOf to extract the number.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode265
src/json.js:265: builder.push(($isFinite(value) ? $String(value) : "null"));
%_NumberToString here too.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270
src/json.js:270: } else { // Regular non-wrapped object
I would put the comment on the next line instead.

[sandholm, 2010-12-06 11:42:31]

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

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode213
src/json.js:213: if (builder.pop() != ",") {
I added a comment.
On 2010/12/05 15:09:06, Erik Corry wrote:
> This seems a bit indirect.  I think it would be clearer to just say if (len ==
> 0).  Is this version faster?

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode229
src/json.js:229: builder.push(%QuoteJSONString(p), ":");
I experimented a bit. Will submit another change.
On 2010/12/05 15:09:06, Erik Corry wrote:
> If you didn't already try it I think it would be interesting to experiment
with
> having QuoteJSONString not add the quotes.  We should be faster at adding the
> quotes now and it would open up the possibility of just returning the string
> unchanged which should be the common case.  We can save that for another
change
> though.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode241
src/json.js:241: if (builder.pop() != ",") {
I added a comment
On 2010/12/05 15:09:06, Erik Corry wrote:
> Again, I find this convoluted.  A boolean variable that gets set if the for
loop
> is non-empty would be clearer.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode258
src/json.js:258: builder.push(($isFinite(value) ? $String(value) : "null"));
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
> Use %_NumberToString instead of the generic $String. You know it's a number.
(Or
> use NonStringToString, but it will just test for being a number and call
> %_NumberToString)

Done.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode258
src/json.js:258: builder.push(($isFinite(value) ? $String(value) : "null"));
On 2010/12/05 15:09:06, Erik Corry wrote:
> I wonder how $String compares speedwise to ("" + value).

Done.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode264
src/json.js:264: value = $Number(value);
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
> use %_ValueOf to extract the number.

Done.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode265
src/json.js:265: builder.push(($isFinite(value) ? $String(value) : "null"));
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
> %_NumberToString here too.

Done.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270
src/json.js:270: } else { // Regular non-wrapped object
On 2010/12/05 15:09:06, Erik Corry wrote:
> There should be 2 spaces before //

Done.

http://codereview.chromium.org/5567005/diff/1/src/json.js#newcode270
src/json.js:270: } else { // Regular non-wrapped object
On 2010/12/06 08:48:45, Lasse Reichstein wrote:
> I would put the comment on the next line instead.

Done.