Changing blob to use arc pointers in order to limit wasteful copying … #8860

craftytrickster · 2015-12-06T18:57:40Z

SimonSapin · 2015-12-06T22:54:15Z

components/script/dom/blob.rs

-    // simpler to use version of read_out_buffer
-    pub fn clone_bytes(&self) -> Vec<u8> {
-        self.bytes.clone().unwrap_or(vec![])
+        self.bytes[start..end].to_vec()


This (.to_vec()) still copies bytes. It looks like this method is used to send Vec<u8> messages, so it probably can’t simply return &[u8] instead.

To reduce copying, the Vec<u8> messages should probably be changed to some reference counted thing. Maybe tendrils? https://github.com/servo/tendril

CC @jdm

I don't know much about tendril and it doesn't really have any good examples of usage afaict. I'm not sure I see any options here; the websocket library accepts a Vec<u8>, so I believe we're going to have to copy at some point here.

Hmm, I'm wrong - the library does accept &[u8], so I think we should make this return &[u8] instead.

jdm · 2015-12-07T02:30:04Z

So looking at the code in filereader.rs, I see we'll need another change to avoid copying while dealing with slices appropriately. I propose we extract the buffer and slice data into a separate structure (BlobSlice) that can be sent between threads, and has a method that returns the actual slice (-> &[u8]). This struct can be the return value of get_bytes() on Blob. Does that make sense?

craftytrickster · 2015-12-08T00:43:26Z

@jdm that does make sense, I'll see what I can do.

craftytrickster · 2015-12-08T02:26:16Z

I pushed a new test commit to show an approach to avoid using to_vec in the get_bytes method.

However, I do not know if I like the decision to split of the DataSlice/BlobSlice into its own struct, it does not feel very ergonomic to me. I'd prefer to keep those fields in the Blob struct. I guess you can look at the latest commit to see if you agree or not. Also, splitting up the struct caused me to get compiler errors on DataSlice unless I implemented heap_size_of_children and trace. I was not sure how to remove these errors, so for the prototype I just kept empty implementations.

jdm · 2015-12-08T06:50:12Z

For what it's worth, that looks like what I was expecting :) I agree that it would be nice to keep the representation of the data within the containing blob, but our current design forbids us from being able to access the internals of JS-owned values (such as Blobs) on other threads.

bors-servo · 2015-12-08T16:34:01Z

☔ The latest upstream changes (presumably #8867) made this pull request unmergeable. Please resolve the merge conflicts.

nox · 2015-12-10T15:35:24Z

@jdm What if we had something like:

pub enum BlobData {
    Owned(Vec<u8>),
    Borrowed(JS<Blob>, usize, usize)
}

This would allow us to actually track the memory usage of the blobs (by making BlobData's HeapSizeOf implementation return 0 for Borrowed values).

jdm · 2015-12-10T15:54:52Z

@nox I'm not sure that really helps if the original blob is inaccessible and only the slice is holding it alive.

Ms2ger · 2015-12-10T16:58:26Z

Another thing to keep in mind is that you can postMessage() a Blob to a worker.

nox · 2015-12-10T17:28:55Z

Oh you are completely right, silly me.

bors-servo · 2015-12-11T00:52:41Z

☔ The latest upstream changes (presumably #8922) made this pull request unmergeable. Please resolve the merge conflicts.

craftytrickster · 2016-01-05T14:52:38Z

Hi @KiChjang, I am still waiting for any feedback on what needs to be done here. If you have any comments, please let me know

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

craftytrickster · 2016-01-05T14:57:31Z

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/blob.rs, line 101 [r1] (raw file):
I am no longer doing to_vec and copying the bytes, I am simply passing a reference to the datsaslice now

Comments from the review on Reviewable.io

jdm · 2016-01-05T14:59:02Z

Sorry this fell through the cracks!

jdm · 2016-01-05T19:04:33Z

This looks like what I expected! There are a couple changes we can make to improve clarity, and I think we might be able to avoid duplicating the blob contents in the websocket code, though.
-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 5 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/script/dom/blob.rs, line 106 [r3] (raw file):
On second thought, let's extend the new constructor to accept bytes_start and bytes_end instead of duplicating this.

components/script/dom/filereader.rs, line 359 [r3] (raw file):
We don't need a sender and receiver any longer. We can store the result of blob.get_data().clone() in a variable and pass that to perform_annotated_read_operation.

components/script/dom/filereader.rs, line 410 [r3] (raw file):
blob_contents should be a DataSlice value instead of a Receiver.

components/script/dom/websocket.rs, line 389 [r3] (raw file):
Can we get rid of the to_vec here?

Comments from the review on Reviewable.io

bors-servo · 2016-01-06T02:34:04Z

☔ The latest upstream changes (presumably #9160) made this pull request unmergeable. Please resolve the merge conflicts.

craftytrickster · 2016-01-06T14:42:56Z

@jdm thank you for the notes, I will take a look at them when I have time tonight and make the appropriate changes


        Changed blob to use DataSlice with Arc in order to limit wasteful cop…

…ying of byte vector

craftytrickster · 2016-01-07T00:42:19Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/script/dom/blob.rs, line 106 [r3] (raw file):
Originally, I tried that, but I was having issues compiling with the use of "box". It seems to work fine now though, so I was able to remove the duplicated function body.

However, since new implies a fresh object, it seems awkward to have to specified None, None, every single time in the constructor. I think we should still keep a separated new_sliced function to avoid this.

If you do not agree, I can simply converge the new & new_sliced together.

components/script/dom/websocket.rs, line 389 [r3] (raw file):
This is a bit complicated, because we would have to change the net_traits lib.rs file that contains the MessageData enum. It would have to be changed to Blob instead of a Vec in order to avoid using to_vec.

The problem is that we cannot import (script) Blob there, because it would create a circular dependency. I am not sure what you would like to do here. Should we move blob to another crate?

Comments from the review on Reviewable.io

jdm · 2016-01-07T07:26:52Z

@bors-servo: r+
Thanks for doing this work!

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/websocket.rs, line 389 [r3] (raw file):
Oh right, I forgot that this became more complicated recently. Nothing we can do here!

Comments from the review on Reviewable.io

bors-servo · 2016-01-07T07:26:53Z

📌 Commit 76f689c has been approved by jdm

bors-servo · 2016-01-07T07:37:42Z

⌛ Testing commit 76f689c with merge 0fcee73...


        Auto merge of #8860 - craftytrickster:8801/blob-vec, r=jdm

Changing blob to use arc pointers in order to limit wasteful copying … Fixes #8801.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8860)

bors-servo · 2016-01-07T08:30:56Z

💔 Test failed - mac-rel-css

KiChjang · 2016-01-07T09:02:23Z

@bors-servo retry

#9163

bors-servo · 2016-01-07T09:05:34Z

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-wpt are reusable. Rebuilding only mac-rel-css...

bors-servo · 2016-01-07T09:40:21Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Dec 6, 2015

SimonSapin reviewed Dec 6, 2015
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 7, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 8, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Dec 8, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 8, 2015

craftytrickster force-pushed the craftytrickster:8801/blob-vec branch from cdafb6f to fea9cd9 Dec 9, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 9, 2015

craftytrickster force-pushed the craftytrickster:8801/blob-vec branch 2 times, most recently from d491137 to a280078 Dec 9, 2015

highfive added the S-needs-rebase label Dec 11, 2015

craftytrickster force-pushed the craftytrickster:8801/blob-vec branch from a280078 to 4f33b07 Dec 11, 2015

highfive removed the S-needs-rebase label Dec 11, 2015

jdm self-assigned this Jan 5, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Jan 5, 2016

highfive added the S-needs-rebase label Jan 6, 2016

Changed blob to use DataSlice with Arc in order to limit wasteful cop…

Loading status checks…

76f689c

…ying of byte vector

craftytrickster force-pushed the craftytrickster:8801/blob-vec branch from 4f33b07 to 76f689c Jan 7, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jan 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Jan 7, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 7, 2016

bors-servo merged commit 76f689c into servo:master Jan 7, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors-servo mentioned this pull request Jan 7, 2016

Support synchronous about:blank loads #8600

Closed

SimonSapin removed the S-tests-failed label Jan 27, 2016

servo / servo

Changing blob to use arc pointers in order to limit wasteful copying … #8860

Changing blob to use arc pointers in order to limit wasteful copying … #8860

craftytrickster commented Dec 6, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Dec 7, 2015

craftytrickster commented Dec 8, 2015

craftytrickster commented Dec 8, 2015

jdm commented Dec 8, 2015

bors-servo commented Dec 8, 2015

nox commented Dec 10, 2015

jdm commented Dec 10, 2015

Ms2ger commented Dec 10, 2015

nox commented Dec 10, 2015

bors-servo commented Dec 11, 2015

craftytrickster commented Jan 5, 2016

craftytrickster commented Jan 5, 2016

jdm commented Jan 5, 2016

jdm commented Jan 5, 2016

bors-servo commented Jan 6, 2016

craftytrickster commented Jan 6, 2016

craftytrickster commented Jan 7, 2016

jdm commented Jan 7, 2016

bors-servo commented Jan 7, 2016

bors-servo commented Jan 7, 2016

bors-servo commented Jan 7, 2016

KiChjang commented Jan 7, 2016

bors-servo commented Jan 7, 2016

bors-servo commented Jan 7, 2016

servo / servo

Join GitHub today

Changing blob to use arc pointers in order to limit wasteful copying … #8860

Changing blob to use arc pointers in order to limit wasteful copying … #8860

Conversation

craftytrickster commented Dec 6, 2015

This comment has been minimized.

SimonSapin Dec 6, 2015

This comment has been minimized.

jdm Dec 7, 2015

This comment has been minimized.

jdm Dec 7, 2015

jdm commented Dec 7, 2015

craftytrickster commented Dec 8, 2015

craftytrickster commented Dec 8, 2015

jdm commented Dec 8, 2015

bors-servo commented Dec 8, 2015

nox commented Dec 10, 2015

jdm commented Dec 10, 2015

Ms2ger commented Dec 10, 2015

nox commented Dec 10, 2015

bors-servo commented Dec 11, 2015

craftytrickster commented Jan 5, 2016

craftytrickster commented Jan 5, 2016

jdm commented Jan 5, 2016

jdm commented Jan 5, 2016

bors-servo commented Jan 6, 2016

craftytrickster commented Jan 6, 2016

craftytrickster commented Jan 7, 2016

jdm commented Jan 7, 2016

bors-servo commented Jan 7, 2016

bors-servo commented Jan 7, 2016

bors-servo commented Jan 7, 2016

KiChjang commented Jan 7, 2016

bors-servo commented Jan 7, 2016

bors-servo commented Jan 7, 2016