Use serde_bytes crate #2598

jonleighton · 2018-04-01T09:04:46Z

This speeds up serializing and deserializing of binary data.

This change is

jonleighton · 2018-04-01T09:05:27Z

Cargo.lock

@@ -395,7 +395,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"

 [[package]]
 name = "freetype"
-version = "0.3.0"
+version = "0.4.0"


This update happened automatically, I believe it is due to d162942

Doesn't webrender have a CI check for that ?

I guess not? Though I don't know, I haven't contributed to webrender before. I've opened a separate PR to fix this: #2599

jonleighton · 2018-04-01T09:06:37Z

webrender_api/src/image.rs

    External(ExternalImageData),
 }

+mod serde_image_data_raw {


This is necessary because we are dealing with an Arc. However, maybe it would be better to patch serde_bytes to support this?

Cc @dtolnay

Could you propose a more general signature for serde_bytes that could work here?

Perhaps not, though I haven't looked into it very closely. Assuming a more general signature is not workable, maybe serde_bytes could also ship serde_bytes_rc/serde_bytes_arc modules though? (Essentially what I've done here, but inside the serde_bytes crate.)

dtolnay · 2018-04-01T10:21:08Z

webrender_api/src/image.rs

+    use serde::{Deserializer, Serializer};
+
+    pub fn serialize<'a, S: Serializer>(bytes: &'a Arc<Vec<u8>>, serializer: S) -> Result<S::Ok, S::Error> {
+        serde_bytes::serialize(&bytes.as_slice(), serializer)


There was a missing T: ?Sized in serde_bytes::serialize. I fixed it in 0.10.4 so the & on this line should no longer be necessary.

jrmuizel · 2018-04-01T18:15:31Z

It seems like this should only be needed when using ipc-channel. i.e. not with Gecko

…

On Sun, Apr 1, 2018, 5:04 AM Jon Leighton ***@***.***> wrote: This speeds up serializing and deserializing of binary data. ------------------------------ You can view, comment on, or merge this pull request online at: #2598 Commit Summary - Use serde_bytes crate File Changes - *M* Cargo.lock <https://github.com/servo/webrender/pull/2598/files#diff-0> (16) - *M* webrender_api/Cargo.toml <https://github.com/servo/webrender/pull/2598/files#diff-1> (1) - *M* webrender_api/src/api.rs <https://github.com/servo/webrender/pull/2598/files#diff-2> (8) - *M* webrender_api/src/image.rs <https://github.com/servo/webrender/pull/2598/files#diff-3> (21) Patch Links: - https://github.com/servo/webrender/pull/2598.patch - https://github.com/servo/webrender/pull/2598.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2598>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUTbfeBMX-aN9cksBPJVA553xsIDJIrks5tkJgugaJpZM4TCxBX> .


        Use serde_bytes crate

This speeds up serializing and deserializing of binary data.

glennw · 2018-04-04T01:12:51Z

@jrmuizel It looks like this would be used even in Gecko (due to the changes to FontData and the Blob structs), wouldn't it?

Is there any issue with having this enabled for Gecko?

Do we need to do anything further here (benchmarks?) or is this ready to merge?

jonleighton · 2018-04-04T01:24:15Z

To give a bit of context to this, I discovered it while profiling the code I was working on for servo/servo#20506

While profiling that code, I was seeing lots of time being spent serializing and deserializing font data bytes when passing them over an IPC channel. I tried using serde_bytes there (i.e. for FontTemplateData in Servo) and it made a big improvement, and I could then see that there was some similar overhead happening when passing bytes to WebRender. I tried building Servo with a local copy of WebRender which used serde_bytes and again saw an improvement when profiling.

(I haven't included serde_bytes in servo/servo#20506 for now since servo/servo@5d5999d mostly negated the need for it and I didn't want to make that PR more complex than it needed to be, but I was still intending to follow up with a PR adding serde_bytes to Servo.)

glennw · 2018-04-04T01:30:40Z

@jonleighton Thanks for the info! What does this mean in terms of the Gecko integration which doesn't use the ipc-channel feature in WR?

Does it have any effect at all? Should we be only enabling it in WR when the ipc-channel feature is enabled (e.g. does it increase the binary size for no performance win in that use case)?

jonleighton · 2018-04-04T01:44:42Z

If the Gecko integration never serializes/deserializes, then yeah, I guess this code has no effect there.

However, wouldn't the compiler be able to figure out that the code is not used and exclude it from the binary? I'm a bit out of my depth here so I don't know if that is really the case, just struck me as something that might happen.

If that isn't the case, then should WebRender exclude the derive(Deserialize, Serialize) attributes when the ipc feature is not enabled?

jonleighton · 2018-04-04T01:45:24Z

If that isn't the case, then should WebRender exclude the derive(Deserialize, Serialize) attributes when the ipc feature is not enabled?

I.e. don't they also bloat the binary?

glennw · 2018-04-04T01:49:55Z

Well, Gecko does serialize / deserialize things (such as the display list) - but they are sent over the Gecko IPC mechanism, rather than the rust ipc-channel crate.

So, I guess the question is does this change provide a benefit in that situation? I would think yes, but I just want to make sure that's the case, since @jrmuizel seems to suggest it's only useful if using ipc-channel, per the comment above.

jonleighton · 2018-04-04T01:59:07Z

Right, so I also think the answer is yes since serde_bytes speeds up the serializing and deserializing of the data, irrespective of how the serialized data is then moved around.

glennw · 2018-04-04T02:15:56Z

That makes sense to me. @jrmuizel Any issues with getting this merged?

jrmuizel · 2018-04-04T02:54:30Z

Gecko doesn't serialize those structs in normal usage, only the display list. That being said, we probably do serialize them when doing a binary recording (we should probably only support this on Nightly)

I guess it's probably ok to take the extra dependency for now and have a feature flag to disable recording support in a follow up.

glennw · 2018-04-04T03:36:17Z

Sounds good, thanks!

@bors-servo r+

bors-servo · 2018-04-04T03:36:18Z

📌 Commit 74d05cc has been approved by glennw

bors-servo · 2018-04-04T03:36:21Z

⌛ Testing commit 74d05cc with merge 270e1e9...


        Auto merge of #2598 - jonleighton:serde_bytes, r=glennw

Use serde_bytes crate This speeds up serializing and deserializing of binary data.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2598)

bors-servo · 2018-04-04T04:23:04Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing 270e1e9 to master...

jonleighton reviewed Apr 1, 2018

View changes

dtolnay reviewed Apr 1, 2018

View changes

Use serde_bytes crate

Loading status checks…

74d05cc

This speeds up serializing and deserializing of binary data.

jonleighton force-pushed the jonleighton:serde_bytes branch from ef80860 to 74d05cc Apr 3, 2018

bors-servo merged commit 74d05cc into servo:master Apr 4, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

servo / webrender

Use serde_bytes crate #2598

Use serde_bytes crate #2598

jonleighton commented Apr 1, 2018 •

edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jrmuizel commented Apr 1, 2018

glennw commented Apr 4, 2018

jonleighton commented Apr 4, 2018

glennw commented Apr 4, 2018

jonleighton commented Apr 4, 2018

jonleighton commented Apr 4, 2018

glennw commented Apr 4, 2018

jonleighton commented Apr 4, 2018

glennw commented Apr 4, 2018

jrmuizel commented Apr 4, 2018

glennw commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

servo / webrender

Join GitHub today

Use serde_bytes crate #2598

Use serde_bytes crate #2598

Conversation

jonleighton commented Apr 1, 2018 • edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jrmuizel commented Apr 1, 2018

glennw commented Apr 4, 2018

jonleighton commented Apr 4, 2018

glennw commented Apr 4, 2018

jonleighton commented Apr 4, 2018

jonleighton commented Apr 4, 2018

glennw commented Apr 4, 2018

jonleighton commented Apr 4, 2018

glennw commented Apr 4, 2018

jrmuizel commented Apr 4, 2018

glennw commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

bors-servo commented Apr 4, 2018

jonleighton commented Apr 1, 2018 •

edited by larsbergstrom