Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUse an unsafe serializer. #1799
Conversation
|
r? @gankro |
dtolnay left a comment
|
Nicely done! Please document more clearly what makes this unsafe. By using this implementation, we are trusting all of the Serialize impls that it ever serializes. I can break the trust with a Serialize like this: struct S {
first: Cell<bool>,
}
impl Serialize for S {
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where S: Serializer
{
if self.first.get() {
self.first.set(false);
().serialize(serializer)
} else {
0.serialize(serializer)
}
}
} |
|
I'm currently building a rustc with the noalias flag we discussed, would like to hold off on merging that until we've looked at the codegen of the 4 combos of |
|
impl otherwise seems fine, with mine and dtolnay's requested doc fixes. |
| @@ -483,6 +485,48 @@ impl<'a, 'b> Serialize for DisplayItemRef<'a, 'b> { | |||
| } | |||
| } | |||
|
|
|||
| // This is a replacement for bincode::serialize_into(&vec) | |||
| // The default implementation Write for Vec will basically | |||
| // call extend_from_slice(). Serde ends up calling that for ever | |||
This comment has been minimized.
This comment has been minimized.
|
Some codgen tests here. Looks like this is an all around win, and even positions us for bigger wins with mutable noalias: |
|
@bors-servo r+ |
|
@gankro: |
This gives a noticable impact on serialization performance in Gecko. wr_dp_push_text() goes from 35.6% of nsDisplayText::CreateWebRenderCommands down to 24%. The generated code is still pretty bad but hopefully adding proper noalias information to rust will fix that.
| fn serialize_fast<T: Serialize>(vec: &mut Vec<u8>, e: &T) { | ||
| // manually counting the size is faster than vec.reserve(bincode::serialized_size(&e) as usize) for some reason | ||
| let mut size = SizeCounter(0); | ||
| bincode::serialize_into(&mut size,e , bincode::Infinite).unwrap(); |
This comment has been minimized.
This comment has been minimized.
| unsafe { | ||
| let old_len = self.0.len(); | ||
| self.0.set_len(old_len + buf.len()); | ||
| ptr::copy_nonoverlapping(buf.as_ptr(), self.0.as_mut_ptr().offset(old_len as isize), buf.len()); |
This comment has been minimized.
This comment has been minimized.
|
@bors-servo r=Gankro,kvark |
|
|
Use an unsafe serializer. This gives a noticable impact on serialization performance in Gecko. wr_dp_push_text() goes from 35.6% of nsDisplayText::CreateWebRenderCommands down to 24%. The generated code is still pretty bad but hopefully adding proper noalias information to rust will fix that. <!-- 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/1799) <!-- Reviewable:end -->
|
|
Some reviews in WR that Gankro has worked on: servo/webrender#1830 servo/webrender#1799 servo/webrender#1834 servo/webrender#1664
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 -->
jrmuizel commentedOct 3, 2017
•
edited by larsbergstrom
This gives a noticable impact on serialization performance in Gecko.
wr_dp_push_text() goes from 35.6% of
nsDisplayText::CreateWebRenderCommands down to 24%.
The generated code is still pretty bad but hopefully adding
proper noalias information to rust will fix that.
This change is