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

src/json.js

Index: src/json.js
===================================================================
--- src/json.js	(revision 5916)
+++ src/json.js	(working copy)
@@ -195,75 +195,96 @@
   }
 }
 
-function BasicSerializeArray(value, stack) {
+
+function BasicSerializeArray(value, stack, builder) {
   if (StackContains(stack, value)) {
     throw MakeTypeError('circular_structure', []);
   }
   stack.push(value);
-  var partial = [];
+  builder.push("[");
   var len = value.length;
   for (var i = 0; i < len; i++) {
-    var strP = BasicJSONSerialize($String(i), value, stack);
-    if (IS_UNDEFINED(strP)) strP = "null";
-    partial.push(strP);
+    var before = builder.length;
+    BasicJSONSerialize($String(i), value, stack, builder);
+    if (before == builder.length) builder.push("null");
+    builder.push(",");

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

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.

   }
   stack.pop();
-  return "[" + partial.join() + "]";
+  if (builder.pop() != ",") {

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

This seems a bit indirect.  I think it would be clearer to just say if (len ==
0).  Is this version faster?

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

I added a comment.
On 2010/12/05 15:09:06, Erik Corry wrote:

+    builder.push("[]");
+  } else {
+    builder.push("]");
+  }
 }
 
-function BasicSerializeObject(value, stack) {
+
+function BasicSerializeObject(value, stack, builder) {
   if (StackContains(stack, value)) {
     throw MakeTypeError('circular_structure', []);
   }
   stack.push(value);
-  var partial = [];
+  builder.push("{");
   for (var p in value) {
     if (ObjectHasOwnProperty.call(value, p)) {
-      var strP = BasicJSONSerialize(p, value, stack);
-      if (!IS_UNDEFINED(strP)) partial.push(%QuoteJSONString(p) + ":" + strP);
+      builder.push(%QuoteJSONString(p), ":");

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

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.

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

I experimented a bit. Will submit another change.
On 2010/12/05 15:09:06, Erik Corry wrote:

+      var before = builder.length;
+      BasicJSONSerialize(p, value, stack, builder);
+      if (before == builder.length) {
+        builder.pop();
+        builder.pop();
+      } else {
+        builder.push(",");
+      }
     }
   }
   stack.pop();
-  return "{" + partial.join() + "}";
+  if (builder.pop() != ",") {

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

Again, I find this convoluted.  A boolean variable that gets set if the for loop
is non-empty would be clearer.

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

I added a comment
On 2010/12/05 15:09:06, Erik Corry wrote:

+    builder.push("{}");
+  } else {
+    builder.push("}");
+  }
 }
 
-function BasicJSONSerialize(key, holder, stack) {
+
+function BasicJSONSerialize(key, holder, stack, builder) {
   var value = holder[key];
   if (IS_OBJECT(value) && value) {
     var toJSON = value.toJSON;
     if (IS_FUNCTION(toJSON)) value = toJSON.call(value, key);
   }
-  // Unwrap value if necessary
-  if (IS_OBJECT(value)) {
+  if (IS_STRING(value)) {
+    builder.push(%QuoteJSONString(value));
+  } else if (IS_NUMBER(value)) {
+    builder.push(($isFinite(value) ? $String(value) : "null"));

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

I wonder how $String compares speedwise to ("" + value).

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

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)

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

Done.

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

Done.

+  } else if (IS_BOOLEAN(value)) {
+    builder.push((value ? "true" : "false"));
+  } else if (IS_OBJECT(value)) {
+    // Unwrap value if necessary
     if (IS_NUMBER_WRAPPER(value)) {
       value = $Number(value);

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

use %_ValueOf to extract the number.

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

Done.

+      builder.push(($isFinite(value) ? $String(value) : "null"));      

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

%_NumberToString here too.

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

Done.

     } else if (IS_STRING_WRAPPER(value)) {
-      value = $String(value);
+      builder.push(%QuoteJSONString($String(value)));
     } else if (IS_BOOLEAN_WRAPPER(value)) {
-      value =  %_ValueOf(value);
-    }
-  }
-  switch (typeof value) {
-    case "string":
-      return %QuoteJSONString(value);
-    case "object":
+      builder.push((%_ValueOf(value) ? "true" : "false")); 
+    } else { // Regular non-wrapped object

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

There should be 2 spaces before //

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

I would put the comment on the next line instead.

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

Done.

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

Done.

       if (!value) {
-        return "null";
+        builder.push("null");
       } else if (IS_ARRAY(value)) {
-        return BasicSerializeArray(value, stack);
+        BasicSerializeArray(value, stack, builder);
       } else {
-        return BasicSerializeObject(value, stack);
+        BasicSerializeObject(value, stack, builder);
       }
-    case "number":
-      return $isFinite(value) ? $String(value) : "null";
-    case "boolean":
-      return value ? "true" : "false";
+    }
   }
 }
 
 function JSONStringify(value, replacer, space) {
   if (IS_UNDEFINED(replacer) && IS_UNDEFINED(space)) {
-    return BasicJSONSerialize('', {'': value}, []);
+    var builder = [];
+    BasicJSONSerialize('', {'': value}, [], builder);
+    if (builder.length == 0) return;
+    else return %StringBuilderConcat(builder, builder.length, "");
   }
   if (IS_OBJECT(space)) {
     // Unwrap 'space' if it is wrapped