Improve deserialization performance #1834

jrmuizel · 2017-10-10T02:21:18Z

I haven't yet tested how much this improves deserialization performance but it should be noticeable.

This change is

Gankra

Here's my comments for now; I'll re-review when you've cleaned it up a bit.

Gankra · 2017-10-10T14:10:32Z

webrender_api/src/display_list.rs

+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        unsafe {
+            if self.buf.offset(buf.len() as isize) > self.end {
+                panic!();


Definitely need a message.

Also the presence of two bufs here is confusing. (I stared at this way too long)

Gankra · 2017-10-10T14:10:32Z

webrender_api/src/display_list.rs

@@ -179,6 +179,45 @@ fn skip_slice<T: for<'de> Deserialize<'de>>(
    (range, count)
 }

+struct UnsafeReader<'a: 'b, 'b> {


Needs docs for why it's unsafe, and why that's fine.

Also why it's desirable.

jrmuizel · 2017-10-10T17:05:50Z

With this version and an updated bincode (servo/bincode#207) I go from 140fps to 200fps on my gmail yaml


        Update bincode so that read_exact will work.

Gankra · 2017-10-10T19:50:40Z

webrender_api/src/display_list.rs

@@ -525,6 +527,73 @@ fn serialize_fast<T: Serialize>(vec: &mut Vec<u8>, e: &T) {
    debug_assert!(((w.0 as usize) - (vec.as_ptr() as usize)) == vec.len());
 }

+// This uses a (start, end) representation instead of (start, len) so that
+// only need to update a single field as we read through it. This gives


"gives" at end of line is junk

Gankra · 2017-10-10T19:50:40Z

webrender_api/src/display_list.rs

+        unsafe {
+            if self.start.offset(buf.len() as isize) > self.end {
+                panic!();
+            }


This if should just be

assert!(self.start.offset(buf.len() as isize) <= self.end, "UnsafeReader: wrote past end of target");

Gankra · 2017-10-10T19:50:40Z

webrender_api/src/display_list.rs

+// only need to update a single field as we read through it. This gives
+// makes it easier for llvm to understand what's going on. (https://github.com/rust-lang/rust/issues/45068)
+// we update the slice only once we're done reading
+struct UnsafeReader<'a: 'b, 'b> {


This is actually totally save to use, so maybe just ExactReader?

glennw · 2017-10-10T20:19:54Z

Wow, that's a nice FPS improvement!

Gankra · 2017-10-10T20:34:20Z

r=me with requested changes


        Improve deserialization performance with a custom Read implementation

With this change I go from 140fps to 200fps on my gmail yaml.

jrmuizel · 2017-10-10T21:32:02Z

@bors-servo r=Gankro

bors-servo · 2017-10-10T21:32:03Z

📌 Commit 9818e4d has been approved by Gankro

bors-servo · 2017-10-10T21:32:06Z

⌛ Testing commit 9818e4d with merge aee99d4...


        Auto merge of #1834 - jrmuizel:unsafe-reader, r=Gankro

Improve deserialization performance I haven't yet tested how much this improves deserialization performance but it should be noticeable.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1834)

bors-servo · 2017-10-10T23:36:28Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Gankro
Pushing aee99d4 to master...

hherman1 · 2017-10-20T05:29:13Z

Hi! Rust noob here,

Would it make sense to make this available in the rust stl if it’s so much faster than the built in?


        Add Gankro to WR reviewers list.

Some reviews in WR that Gankro has worked on: servo/webrender#1830 servo/webrender#1799 servo/webrender#1834 servo/webrender#1664


        Auto merge of #740 - glennw:rev-update, r=larsbergstrom

Add Gankro to WR reviewers list. Some reviews in WR that Gankro has worked on: servo/webrender#1830 servo/webrender#1799 servo/webrender#1834 servo/webrender#1664  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/740)

Gankra · 2017-10-20T14:32:21Z

Probably not, for a few reasons. It's specific to how we're using serde (which isn't part of std), and is also unsound if you're not using serde exactly like us. Basically we're relying on the fact that all of our serialization code is machine-generated, so we can't ever get expected and actual size wrong.

Gankra suggested changes Oct 10, 2017

View changes

jrmuizel force-pushed the jrmuizel:unsafe-reader branch from e0afa31 to 370ff2f Oct 10, 2017

pcwalton mentioned this pull request Oct 10, 2017

Forward read_exact() as well as read(). servo/bincode#207

Merged

jrmuizel changed the title ~~WIP: Improve deserialization performance~~ Improve deserialization performance Oct 10, 2017

jrmuizel force-pushed the jrmuizel:unsafe-reader branch from 370ff2f to fc34ec0 Oct 10, 2017

Update bincode so that read_exact will work.

d4d96c6

jrmuizel force-pushed the jrmuizel:unsafe-reader branch from fc34ec0 to ea1c1cf Oct 10, 2017

Gankra suggested changes Oct 10, 2017

View changes

Improve deserialization performance with a custom Read implementation

Loading status checks…

9818e4d

With this change I go from 140fps to 200fps on my gmail yaml.

jrmuizel force-pushed the jrmuizel:unsafe-reader branch from ea1c1cf to 9818e4d Oct 10, 2017

glennw pushed a commit to glennw/saltfs that referenced this pull request Oct 20, 2017

Add Gankro to WR reviewers list.

f2a568d

Some reviews in WR that Gankro has worked on: servo/webrender#1830 servo/webrender#1799 servo/webrender#1834 servo/webrender#1664

glennw mentioned this pull request Oct 20, 2017

Add Gankro to WR reviewers list. servo/saltfs#740

Merged

servo / webrender

Improve deserialization performance #1834

Improve deserialization performance #1834

jrmuizel commented Oct 10, 2017 •

edited by larsbergstrom

Gankra left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jrmuizel commented Oct 10, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Oct 10, 2017

Gankra commented Oct 10, 2017

jrmuizel commented Oct 10, 2017

bors-servo commented Oct 10, 2017

bors-servo commented Oct 10, 2017

bors-servo commented Oct 10, 2017

hherman1 commented Oct 20, 2017

Gankra commented Oct 20, 2017

servo / webrender

Join GitHub today

Improve deserialization performance #1834

Improve deserialization performance #1834

Conversation

jrmuizel commented Oct 10, 2017 • edited by larsbergstrom

Gankra left a comment

This comment has been minimized.

Gankra Oct 10, 2017

This comment has been minimized.

Gankra Oct 10, 2017

This comment has been minimized.

Gankra Oct 10, 2017

This comment has been minimized.

Gankra Oct 10, 2017

jrmuizel commented Oct 10, 2017 • edited

This comment has been minimized.

Gankra Oct 10, 2017

This comment has been minimized.

Gankra Oct 10, 2017

This comment has been minimized.

Gankra Oct 10, 2017

glennw commented Oct 10, 2017

Gankra commented Oct 10, 2017

jrmuizel commented Oct 10, 2017

bors-servo commented Oct 10, 2017

bors-servo commented Oct 10, 2017

bors-servo commented Oct 10, 2017

hherman1 commented Oct 20, 2017

Gankra commented Oct 20, 2017

jrmuizel commented Oct 10, 2017 •

edited by larsbergstrom

jrmuizel commented Oct 10, 2017 •

edited