Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink Structured Cloning/Transferring #23917

Closed
gterzian opened this issue Aug 6, 2019 · 0 comments
Closed

Rethink Structured Cloning/Transferring #23917

gterzian opened this issue Aug 6, 2019 · 0 comments

Comments

@gterzian
Copy link
Member

@gterzian gterzian commented Aug 6, 2019

For structured cloning(and soon transferring, see #23637) we're relying on the "StructuredClone" API of Spidermonkey, available via the bindings at https://github.com/servo/servo/blob/ff3a9180a709a18ae1c143b739bbbc0224a5f695/components/script/dom/bindings/structuredclone.rs

It's a really convenient way to clone basic JS objects, which are dealt with directly by the JS engine, giving us a Vec<u8> in return.

It gets much harder when it comes to platform objects that need to be cloned, or transferred, by Servo itself.

An example is the Blob.

The problem is really that the API consists of raw pointers, making it very manual and hard to clone or transfer something. I can't really imagine us having to do this for each new object that needs to be cloneable or transferable.


Experience report

With the ongoing work in MessagePort, I went down the route of only transferring the MessagePortId, which is a u64(actually two u32), and the JS engine conveniently makes a *mut u64 available. So the Id is "easy" to transfer.

It's again much harder to do that with more complicated objects, although the engine makes a *mut *mut raw::c_void available, it's not clear to me how to actually use it. I've tried just casting the entire MessagePort to a raw pointer, and the result were not satisfactory. The same thing went for casting "smaller" objects, like the MessagePortId(a pair of u32), which would give completely different numbers on the transfer receive side.

The only thing I could get to work was manually turning the pair of u32 in a MessagePortId to a u64, and then passing that to the *mut u64 made available by Spidermonkey.

Not being able to transfer more than the MessagePortId meant I had to add a huge layer of glue clode to "complete the transfer" using IPC outside of the structure clone API. It's complicated since that means synchronizing the transfer across event-loops...

If the port could be transferred "as is", the code would be much simpler since the transfer would be synchronized with receiving the message containing the transferred port on the receiving event-loop.

An example of how simple the code can be can be found in Chromium/Blink. And the reason it's simpler is that there the port is transferred directly as part of sending and receiving of an IPC message across event-loops. My version is complicated because I can only transfer the Id, and then need to "sync-up" the transfer across event-loops using the constellation(without blocking the event-loops).


Potential solution

The current message-port work in progress has a MessagePortImpl, which is the data backing the port, and a MessagePort, which is the DOM object.

The MessagePortImpl is the thing that we want to transfer, while the DOM MessagePort is ephemeral in nature and we could say "tied to the JS context where it is found".

Interestingly, Blob also consist of a BlobImpl and a Blob. Which to me points us in the following direction:

  1. When an DOM object is cloneable or transferreable, and that operation is not supported natively by SpiderMonkey, we probably should not rely on the StructuredClone API for it.
  2. Instead, we could try to split the object in two, one Impl and one DomObject part, where the Impl would be Serialize/Deserialize.
  3. In that case the Impl and DomObject need to share an Id created using the pipeline-namespace, since that Id needs to be unique across the UA as it can be transferred from one event-loop to another.
  4. Then, we could transfer or clone it just using serde and ipc-channel.
  5. Since the transfer or cloning is often done as part of postMessage, which can come witha combination of a simple "JS message" and a transferred object, we probably still want to use Spidermonkey to clone the "simple JS message" or other values that are handled by the engine directly.

How to do this? Well, instead of having our whole StructuredCloneData consists of using APIs like JS_WriteStructuredClone, which then potentially call back into Servo to clone or transfer some platform objects, we should turn things around and first try to use serde, and then have our code potentially use JS_WriteStructuredClone if it wants to.

so a call to StructuredCloneData would first look at what is being passed to it, and if it's something like a MessagePort it would grab the corresponding MessagePortImpl, and serialize it just using serde, and if it's something that Servo does not want to serialize itself, it would use SpiderMonkey.

I guess we would need to then split the two serialized data, and keep track of what kind of stuff we serialized so that we know what to do with it on the other side.

So a message could look like:

StructuredCloneSerializedData {
    js: Vec<u8>,
    serde: Vec<u8>,
}

maybe the serde part could actually be an enum, like:

#[derive(Deserialize, Serialize)]
pub enum StructuredSerialized {
    MessagePort(MessagePortImpl),
    Blob(BlobImpl)
}

That way serde would know exactly what to do on both sides?

Anyway, the main idea is stop trying to go through JS_WriteStructuredClone and then callback into Servo, instead do most with serde, and maybe call into JS_WriteStructuredClone for some things.

@gterzian gterzian mentioned this issue Aug 11, 2019
0 of 5 tasks complete
@gterzian gterzian mentioned this issue Sep 2, 2019
0 of 5 tasks complete
bors-servo added a commit that referenced this issue Sep 4, 2019
[WIP] Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of #23917

Depends on the changes to structured cloning introduced in #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 7, 2019
[WIP] Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of #23917

Depends on the changes to structured cloning introduced in #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 8, 2019
[WIP] Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of #23917

Depends on the changes to structured cloning introduced in #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 11, 2019
[WIP] Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of FIX #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 11, 2019
[WIP] Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of FIX #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 11, 2019
[WIP] Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of FIX #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 19, 2019
Restructure Blob, structured serialization

<!-- Please describe your changes on the following line: -->
FIX #24052 and also address the "cloning" half of FIX #23917

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24123)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

1 participant
You can’t perform that action at this time.