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

Make the writer even more unsafe. #1830

Merged
merged 1 commit into from Oct 8, 2017
Merged

Conversation

@jrmuizel
Copy link
Contributor

jrmuizel commented Oct 6, 2017

Instead of changing the length every write we just adjust the pointer.
This avoids rust-lang/rust#45068.
However we now need to ensure that we set the length when we are done.


This change is Reviewable

@@ -523,7 +521,11 @@ fn serialize_fast<T: Serialize>(vec: &mut Vec<u8>, e: &T) {
bincode::serialize_into(&mut size,e , bincode::Infinite).unwrap();
vec.reserve(size.0);

bincode::serialize_into(&mut UnsafeVecWriter(vec), e, bincode::Infinite).unwrap();
let old_len = vec.len();
let ptr = unsafe { vec.as_mut_ptr().offset(vec.len() as isize) };

This comment has been minimized.

Copy link
@Gankra

Gankra Oct 6, 2017

Contributor

r=me with this changed to old_len

@jrmuizel jrmuizel force-pushed the jrmuizel:even-more-unsafe branch from 43705c1 to eaa5d63 Oct 6, 2017
@jrmuizel
Copy link
Contributor Author

jrmuizel commented Oct 6, 2017

I fixed up old_len and added a debug_assert to make sure things are sane.

Instead of changing the length every write we just adjust the pointer.
This avoids rust-lang/rust#45068.
However we now need to ensure that we set the length when we are done.

With this patch only 1.2% of WebRender display list building is spent
in serialization.
@jrmuizel
Copy link
Contributor Author

jrmuizel commented Oct 6, 2017

With this patch only 1.2% of WebRender display list building is spent in serialization.

@jrmuizel jrmuizel force-pushed the jrmuizel:even-more-unsafe branch from eaa5d63 to 94e88a5 Oct 6, 2017
@Gankra
Copy link
Contributor

Gankra commented Oct 7, 2017

Nice!

@pcwalton
Copy link
Collaborator

pcwalton commented Oct 7, 2017

What was the fraction of time spent in serialization before?

@jrmuizel
Copy link
Contributor Author

jrmuizel commented Oct 7, 2017

I have too few samples to get a good comparison of the unsafe serializers. We have some patches coming that get rid of the cpu time spent elsewhere so that should raise the percentage of time spent in serialization. The safe serializer accounted for roughly 5.1% of the display list construction time.

@Gankra
Copy link
Contributor

Gankra commented Oct 7, 2017

r=me, don't have review rights yet

@emilio
Copy link
Member

emilio commented Oct 8, 2017

@bors-servo r=Gankro

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2017

📌 Commit 94e88a5 has been approved by Gankro

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2017

Testing commit 94e88a5 with merge a40e1ca...

bors-servo added a commit that referenced this pull request Oct 8, 2017
Make the writer even more unsafe.

Instead of changing the length every write we just adjust the pointer.
This avoids rust-lang/rust#45068.
However we now need to ensure that we set the length when we are done.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1830)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2017

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

@bors-servo bors-servo merged commit 94e88a5 into servo:master Oct 8, 2017
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
glennw pushed a commit to glennw/saltfs that referenced this pull request Oct 20, 2017
bors-servo added a commit to servo/saltfs that referenced this pull request Oct 20, 2017
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

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.