Use StructuredSerialize/StructuredDeserialize #171
Conversation
| here allows other algorithms in this specification to treat it as | ||
| an ECMAScript value, but implementations may optimize this | ||
| if the difference in behavior is not observable. | ||
| </details> |
There was a problem hiding this comment.
This difference can be observable, especially with very large ArrayBuffer objects. With those the initial copy might succeed, but then a later serialization could fail in a naive implementation. That seems somewhat bad.
There was a problem hiding this comment.
I do think it'd be ideal to operate on the Records in the spec, although it is interesting that Blink doesn't do that.
It would also allow you to get rid of this user-agent-defined Realm business. Part of the motivation for this overhaul was to allow specs to stop doing that sort of thing.
I can understand wanting to land a minimal version that "un-breaks" the spec first though, so working on that as a follow-up seems OK.
index.bs
Outdated
| @@ -28,6 +28,12 @@ spec: html; urlPrefix: https://html.spec.whatwg.org/multipage/ | |||
| urlPrefix: dom.html | |||
| type: interface | |||
| text: Document; url: document | |||
| urlPrefix: infrastructure.html | |||
There was a problem hiding this comment.
All of these should link automatically without having to be added here. Are they not?
There was a problem hiding this comment.
Not as of yesterday... looks like bikeshed has picked them up now, though.
| here allows other algorithms in this specification to treat it as | ||
| an ECMAScript value, but implementations may optimize this | ||
| if the difference in behavior is not observable. | ||
| </details> |
There was a problem hiding this comment.
I do think it'd be ideal to operate on the Records in the spec, although it is interesting that Blink doesn't do that.
It would also allow you to get rid of this user-agent-defined Realm business. Part of the motivation for this overhaul was to allow specs to stop doing that sort of thing.
I can understand wanting to land a minimal version that "un-breaks" the spec first though, so working on that as a follow-up seems OK.
index.bs
Outdated
| @@ -5816,7 +5868,8 @@ follows. | |||
| to=] |key|, then remove the [=object-store/record=] from |store| using the | |||
| steps to [=delete records from an object store=]. | |||
|
|
|||
| 4. Store a record in |store| containing |key| as its key and |value| | |||
| 4. Store a record in |store| containing |key| as its key and | |||
| <a abstract-op>StructuredSerialize</a>(|value|) | |||
There was a problem hiding this comment.
What happens if this throws?
There was a problem hiding this comment.
That should get a ! sigil. At this point there are several layers of spec fiction happening. It's "safe" because this is operating on the clone made into a user-agent-defined Realm which has only been mutated by the previous steps.
In Blink, at least, since these steps are running in a process w/o a JS implementation, we actually do a bunch of the work (computing index keys) earlier, and some of the work (injecting generated keys) lazily when the values are later retrieved! Single-process implementations probably work more like the spec does (or have observable behavior differences/bugs), but I haven't looked at the code for any recently.
@domenic - I'll try and grab time with you to discuss some of this in real-time; I think we're hitting the limits of communicating through the peephole of review comments. :) Obviously the specs should be free of assumptions, but getting some advice on approaches for spec fiction would be swell. That wouldn't be in lieu reviews for actual changes, of course.
inexorabletash
force-pushed
the
serialize
branch
3 times, most recently
from
494e010 to
f1949e6
Compare
inexorabletash
force-pushed
the
serialize
branch
from
f1949e6 to
5618f88
Compare
So far as I can tell, this is the minimum set of changes needed to replace uses of StructuredClone with StructuredSerialize/StructuredDeserialize, per #170
Note that when
put()(etc) is called that this does not simply serialize then operate on the output record -the record is immediately deserialized to a clone in an abstract targetRealm so that subsequent operations (extract a key, inject a key, etc) are not updated. I added non-normative details explaining this at the various sites.
(Somewhat coincidentally, that's how Blink is actually implemented, since the output of serialization is a opaque set of bytes rather than an easily-traversed structure.)
The definition of operations that operate on the clone could be changed to operate on the record instead. This would require redoing the "extract a key from a value" and "inject a key into a value" section (replace "value" with "record"), and duplicating the "convert a key to a value" and "convert a value to a key" steps with "record" variants.