If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Improve efficiency of structured cloning on plain objects/arrays

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
4 years ago
7 months ago

People

(Reporter: bhackett, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 757719 [details] [diff] [review]
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.
(Reporter)

Updated

4 years ago
Attachment #757719 - Attachment is patch: true
Attachment #757719 - Flags: review?(jorendorff)
Unfortunately, that spec does cover the case of [[Get]] mutating the object... it's supposed to work like any enumeration that does that.
(Reporter)

Comment 2

4 years ago
Created attachment 758191 [details] [diff] [review]
patch (78c0e2d241ff)

Fix to have enumeration style behavior in the presence of getters, and add a test for this behavior.
Attachment #757719 - Attachment is obsolete: true
Attachment #757719 - Flags: review?(jorendorff)
Attachment #758191 - Flags: review?(jorendorff)
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 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".
Attachment #758191 - Flags: review?(jorendorff) → review+
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.
(Assignee)

Updated

3 years ago
Assignee: general → nobody
Blocks: 1254240
Jason, Brian, could we get this landed, if it still helps things?
Flags: needinfo?(jorendorff)
Flags: needinfo?(bhackett1024)
I tried importing the patch, but there are conflicts everywhere.

I can't take this on right now.
Flags: needinfo?(jorendorff)
(Reporter)

Comment 8

7 months ago
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.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Flags: needinfo?(bhackett1024)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.