879083 - Improve efficiency of structured cloning on plain objects/arrays

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch (78c0e2d241ff)

The time spent doing structured cloning of JSON objects is a major bottleneck in shifting work from the main thread to workers.  Reducing this is pretty critical for e.g. cutting the main thread overhead of writing the session store file.  One simple way of reducing this is improving the efficiency of the object operations done when writing out plain objects and arrays.  Currently, when handling such an object all its properties are enumerated, then fetched later using the generic VM paths.  It would be faster to bake in the knowledge that the object and array classes don't do anything special, and optimize the cloning traverse algorithm to reenter the rest of the VM as little as possible.  The main issue with this is that the structured cloning algorithm can call getters on the objects it traverses, which could change properties on the object in a way that is visible to the resulting read.  This behavior is pretty insane though and doesn't seem to be covered by the spec linked in jsclone.cpp, so it seems fair to punt on this and throw if a getter tries to modify the accessed object.

The attached patch does this (well, as much as possible --- modifications made by getters to the dense elements can't be detected), and improves a microbenchmark that repeatedly serializes the JSON in my session store file from 370ms to 300ms.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Unfortunately, that spec does cover the case of [[Get]] mutating the object... it's supposed to work like any enumeration that does that.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch (78c0e2d241ff)

Fix to have enumeration style behavior in the presence of getters, and add a test for this behavior.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

A few highlights from the e-mail thread.

We have a benchmark that send a sessionstore.js-like object across workers here:
http://yoric.github.com/Bugzilla-822407/?test=sendObject

The corresponding profile is here:
http://people.mozilla.com/~bgirard/cleopatra/#report=d2eb752eaac5af4d515fa6329a8571d330b8b266

I see 61.4% spent in JSStructuredCloneWriter::write, including 
27.9% spent in JSStructuredCloneWriter::writeString.

I suspect that zero-copy string would decrease these 27.9% a lot.
Where's the rest of these 61.4%?

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 758191 [details] [diff] [review]
patch (78c0e2d241ff)

Review of attachment 758191 [details] [diff] [review]:
-----------------------------------------------------------------

This is nice. I'm sorry for the slow review.

::: js/src/jit-test/tests/basic/serialization-getters.js
@@ +1,5 @@
> +
> +/* Test hijinks related to objects rearranging themselves during the middle of serialization. */
> +
> +var b = {};
> +Object.defineProperty(b, "d", {get: function() { return 3; }, enumerable: true});

You could just write:

    var b = {get d() { return 3; }};

@@ +3,5 @@
> +
> +var b = {};
> +Object.defineProperty(b, "d", {get: function() { return 3; }, enumerable: true});
> +var a = deserialize(serialize(b));
> +assertEq(JSON.stringify(a), JSON.stringify({d:3}));

If you wanted to, you could write:

    load(libdir + "asserts.js");

    assertDeepEq(a, {d: 3});

and the same for the array below. But JSON.stringify is plenty good enough for me.

::: js/src/jsclone.cpp
@@ +645,1 @@
>          return false;

js_delete(entry) first.

@@ +769,5 @@
> +    if (state == DenseElements) {
> +        for (size_t i = 0; i < object->getDenseInitializedLength(); i++) {
> +            if (i == index)
> +                newIndex = identifiers.length();
> +            if (!object->getDenseElement(i).isMagic(JS_ELEMENTS_HOLE)) {

Simpler, I think:

    if (state == DenseElements) {
        for (size_t i = index; i < object->getDenseInitializedLength(); i++) {
            if (...) {
                if (!identifiers.append(...))
                    return false;
            }
        }
        newIndex = 0;
    }

Something similar could probably be done when state == Shapes, and then newIndex could be removed entirely; the end of the function would just assign index = 0.

@@ +839,5 @@
> +                }
> +                break;
> +            }
> +            out.writePair(SCTAG_NULL, 0);
> +            deadObjects.append(entry);

Check append() for failure.

::: js/src/jsclone.h
@@ +222,5 @@
> +    };
> +
> +    // All objects currently being written out, and any state for objects
> +    // previously written which are available for reuse.
> +    js::Vector<ObjectStackEntry *, 16, js::SystemAllocPolicy> objects, deadObjects;

I think "objects" and "deadObjects" should have separate declarations and separate comments.

Instead of renaming "objs" to "objects", how about "stack"?

And perhaps "deadObjects" to "stackEntryFreelist".

Comment 5

[Jason Orendorff [:jorendorff]]

Come to think of it, why not use a linked list instead of a Vector? I haven't looked, but maybe both 'stack' and 'stackEntryFreelist' can use the same link field.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Jason, Brian, could we get this landed, if it still helps things?

Comment 7

[Jason Orendorff [:jorendorff]]

I tried importing the patch, but there are conflicts everywhere.

I can't take this on right now.

Comment 8

[Brian Hackett [Laid off!]]

The effort this patch was associated with (writing the session store file OMT) was abandoned a while ago I think.  If structured cloning performance is a bottleneck anywhere it would be best I think to start from scratch in a new bug.