New bincode-based IPC model

[Gankra]

This replaces the existing unsound IPC with new sound and validated bincode-based IPC. On my laptop this regresses mean display list IPC time from 1.5ms to 1.8ms, which is reasonable enough to land and optimize later.

Major differences:

• We never construct a Vec<DisplayItem> any more. DisplayListBuilder directly serializes the data into a Vec. The backend similarly deserializes data on-the-fly. This reduces the memory footprint of BuiltDisplayLists, and eliminates several copies/allocations.

• AuxiliaryLists are no more. Auxiliary data is stored in the DisplayList next to the relevant item. When streaming the data out, we skip over these lists and just produce ItemRanges, similar to the old API. To do this while minimally impacting the rest of webrender and its consumers, there are two new "dummy" DisplayItems: SetGradientStops and SetClipRegion. These affect the subsequent DisplayItem, and will never be yielded by the BuiltDisplayListIter.

• DisplayItem no longer stores any ItemRanges or a ClipRegion. Instead these values are stored "on the side" and can be requested from the item yielded by BuiltDisplayListIter. This necessitates threading some additional state for certain methods. Notably number-of-items and size-of-item-range are no longer equivalent, and the former must therefore be threaded separately. ItemRange's fields have been made private to prevent anyone from relying on this.

• ItemRanges are now typed. This makes type declarations clearer, prevents theoretical bugs, and gives DisplayList a nicer API for getting auxiliary data.

• I needed to rewrite some of the gradient code, as it was relying on reverse iteration of auxiliary data, which is no longer supported. @rlhunt has approved my new implementation.

NOTE: this changes some public APIs and will need some changes in Servo and Gecko. I'll work on those follow ups next week.

wr-stats in Servo:

With Transmute (Before This PR):

With Bincode (After This PR):

---

This change is 

[Gankra]

CC @TyOverby -- pretty good results!

[Gankra]

#1182 results

No noticeable difference.

[bors-servo]

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

[Gankra]

rebased -- nothing interesting changed if you had already started reviewing.

[bors-servo]

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

[glennw]

@gankro Sorry for all the merge conflicts - @kvark and I are working through the review of this today :)

[Gankra]

It's probably just imports... alphabetical imports are a friggin nightmare

[glennw]

Reviewed 14 of 19 files at r1, 4 of 4 files at r2.
Review status: 18 of 23 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

webrender/examples/basic.rs, line 196 at r2 (raw file):

fn push_sub_clip(api: &RenderApi, builder: &mut DisplayListBuilder, bounds: &LayoutRect) 
    -> ClipRegionToken {
    

nit: extra space

---

webrender/src/render_backend.rs, line 208 at r2 (raw file):

                                BuiltDisplayList::from_data(data.display_list_data,
                                                            display_list_descriptor);
                            

nit: extra spaces here

---

webrender_traits/Cargo.toml, line 15 at r2 (raw file):

[dependencies]
app_units = "0.4"
bincode = "1.0.0-alpha2"

Should this be alpha-7?

---

webrender_traits/src/channel.rs, line 53 at r2 (raw file):

        payload_reader.read_exact(&mut built_display_list_data[..]).unwrap();

        // TODO(new-ipc): assert_eq!(payload_reader.position(), data.len() as u64);

Can this be re-enabled?

---

wrench/src/yaml_frame_writer.rs, line 481 at r2 (raw file):

        loop {
            if let Some(traversal) = continue_traversal.take() { *list_iterator = traversal; }
            let base = if let Some(base) = list_iterator.next() { base } else { break };

Perhaps use a match here?

---

Comments from Reviewable

[glennw]

Partial review of the simple diffs - more coming.

[glennw]

Reviewed 5 of 19 files at r1.
Review status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.

---

webrender/src/frame_builder.rs, line 405 at r2 (raw file):

                      border_item: &BorderDisplayItem,
                      gradient_stops: ItemRange<GradientStop>,
                      gradient_stops_count: usize) {

Similar to other comments - I'm unsure why a count in addition to range is needed here?

---

webrender/src/prim_store.rs, line 251 at r2 (raw file):

pub struct GradientPrimitiveCpu {
    pub stops_range: ItemRange<GradientStop>,
    pub stops_count: usize,

Why the count here? Isn't it part of the item range?

---

webrender_traits/src/display_item.rs, line 424 at r2 (raw file):

    pub complex_clips: ItemRange<ComplexClipRegion>,
    #[serde(default, skip_serializing, skip_deserializing)]
    pub complex_clip_count: usize,

Why do we need a complex_clip_count here (isn't it available in the ItemRange above)?

---

webrender_traits/src/display_list.rs, line 44 at r2 (raw file):

#[derive(Clone, Default)]
pub struct BuiltDisplayList {
    /// Serde encoded bytes. Mostly DispalyItems, but some mixed in slices.

nit: spelling

---

webrender_traits/src/display_list.rs, line 80 at r2 (raw file):

#[derive(PartialEq)]
enum Peek { StartPeeking, IsPeeking, NotPeeking }

nit: separate lines

---

webrender_traits/src/display_list.rs, line 156 at r2 (raw file):

        use SpecificDisplayItem::*;

        if self.peeking == Peek::IsPeeking {

match might be clearer here. extra space at the end of else clause too.

---

*[webrender_traits/src/display_list.rs, line 171 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6o2zDGLKC8e3WAHVe:-Kj6o2zDGLKC8e3WAHVf:b-rhuq1w) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L171)):*
> ```Rust
>             self.cur_item = bincode::deserialize_from(&mut self.data, bincode::Infinite)
>                                     .expect("MEH: malicious process?");
>     
> ```

nit: extra space

---

*[webrender_traits/src/display_list.rs, line 179 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6oCo-5nYjd2-9PQAJ:-Kj6oCo-5nYjd2-9PQAK:by8qs83) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L179)):*
> ```Rust
>                     self.cur_clip.complex_clips = clip_range;
> 
>                     // This is a dummy item, skip over it
> ```

This might become clearer as I read further - but why the difference in treatment between clip/gradients and glyphs/stacking contexts?

---

*[webrender_traits/src/display_list.rs, line 184 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6oAqQ0P80qVSocI9P:-Kj6oAqQ0P80qVSocI9Q:b-rhuq1w) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L184)):*
> ```Rust
>                 SetGradientStops => {
>                     self.cur_stops = self.skip_slice::<GradientStop>().0;
>                     
> ```

nit: extra space

---

*[webrender_traits/src/display_list.rs, line 196 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6oKFhgkom-IehXm90:-Kj6oKFhgkom-IehXm91:b-3ru1) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L196)):*
> ```Rust
>                 _ => { /* do nothing */ }
>             }
>             
> ```

nit: extra spaces

---

*[webrender_traits/src/display_list.rs, line 339 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6oeez4UIOnUrrHltE:-Kj6oef-ly3lWwsUfyXO:b-3ru1) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L339)):*
> ```Rust
>                                   .expect("MEH: malicious input?")
>         };
>         
> ```

nit: extra spaces

---

*[webrender_traits/src/display_list.rs, line 393 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6opFhAcUkL5Egv6vV:-Kj6opFhAcUkL5Egv6vW:b-3ru1) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L393)):*
> ```Rust
> 
>         map.serialize_entry("item", self.display_item())?;
>         
> ```

nit: extra spaces

---

*[webrender_traits/src/display_list.rs, line 450 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6owZ0ZZaxIYNog4TV:-Kj6owZ0ZZaxIYNog4TW:b-kjrh5a) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L450)):*
> ```Rust
>         let mut temp = BuiltDisplayList::default();
>         ::std::mem::swap(&mut temp.data, &mut self.data);
>         
> ```

spaces

---

*[webrender_traits/src/display_list.rs, line 457 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6oy2fS33HGLBwuns3:-Kj6oy2fS33HGLBwuns4:b-kjrh5a) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L457)):*
> ```Rust
>             }
>         }    
>         
> ```

spaces

---

*[webrender_traits/src/display_list.rs, line 497 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6p7C3OlToGfNUqhMn:-Kj6p7C3OlToGfNUqhMo:b-ivl8dc) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L497)):*
> ```Rust
>     pub fn push_rect(&mut self,
>                      rect: LayoutRect,
>                      _token: ClipRegionToken,
> ```

Why are these here but unused?

---

*[webrender_traits/src/display_list.rs, line 686 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6pGit5zdg5MKC008w:-Kj6pGit5zdg5MKC008x:b6x5q3z) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L686)):*
> ```Rust
>             let last_color = stops.last().unwrap().color;
> 
>             let stops = [
> ```

nit: indentation

---

*[webrender_traits/src/display_list.rs, line 749 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6qjZgy6HJTXBG0lcS:-Kj6qjZgy6HJTXBG0lcT:b-ivl8dc) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L749)):*
> ```Rust
>     pub fn push_border(&mut self,
>                        rect: LayoutRect,
>                        _token: ClipRegionToken,
> ```

Why are these here but unused?

---

*[webrender_traits/src/display_list.rs, line 906 at r2](https://reviewable.io:443/reviews/servo/webrender/1181#-Kj6qvWvvkCPJdYcj-qN:-Kj6qvWvvkCPJdYcj-qO:b28v78g) ([raw file](https://github.com/servo/webrender/blob/14dafa46a0fb30e9c292f20ee9048ebacb53626f/webrender_traits/src/display_list.rs#L906)):*
> ```Rust
>         // because we need to update clip info!
> 
>         // TODO(new-ipc): reimplement this properly (currently losing all aux data)
> ```

Does this need to be fixed up before landing? It's used by some code in gecko at the moment, but nowhere else (I think).

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/servo/webrender/1181)*
<!-- Sent from Reviewable.io -->

[glennw]

@gankro Hmm, the reviewable comments above don't seem to render correctly for me - hopefully they show up OK in reviewable itself.

In general this looks great!

My main questions are related to the item counts that seem to be passed around along with item ranges, and also what the complexities are with how gradient stops are handled compared to glyphs.

There's also a couple of TODO comments that seem like they might need to be resolved before merging.

It'd also be good for @kvark and / or @mrobinson to take a look over this too if they have time :)

[kvark]

Looks good! We discussed the need to pass count and the hack around borrowck in person. I believe this is ready to merge, once:

• the gradient logic is moved in a separate PR to merge before this one
• Glenn's comments are addressed
• CI is green

---

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

---

webrender/doc/CLIPPING.md, line 15 at r2 (raw file):

pub struct ClipRegion {
    pub main: LayoutRect,
    pub complex: ItemRange<ComplexClip>,

looks sweet! 👍

---

webrender_traits/src/display_list.rs, line 214 at r2 (raw file):

        let mut iter = AuxIter::<T>::new(self.data);
        let count = iter.len();
        for _ in &mut iter {}

that's spooky

---

Comments from Reviewable

[mrobinson]

Phwew. This is a big patch. I like that you've been able to eliminate auxiliary_data. I did a once-over review, identify mainly style issues.

[mrobinson]

The indentation style for Servo and WebRender is:

fn push_sub_clip(api: &RenderApi, 
                 builder: &mut DisplayListBuilder, 
                 bounds: &LayoutRect) 
                 -> ClipRegionToken {

[mrobinson]

Indentation here should look like:

api.add_image(mask_image,
              ImageDescriptor::new(2, 2, ImageFormat::A8, true),
              ImageData::new(vec![0, 80, 180, 255]),
              None);

I realize this had the funky indentation before, but this is a good moment to fix it. ☺

[mrobinson]

The code style is a little bit funky here as well.

[mrobinson]

_boo is really cute.

[mrobinson]

This can all be one line. { in WebRender never gets its own line. I guess this can be considered a general comment for the whole PR.

[mrobinson]

This might be better expressed as a match:

let item = match traversal.next() {
    Some(item) => item,
    None => break,
};

[mrobinson]

These lines should be <= 100 characters long.

[mrobinson]

I think it makes sense to add a method to item that returns a tuple containing the gradients_stops and the count together.

[mrobinson]

If this cannot happen any longer, let's add an assertion here that shows it.

[mrobinson]

Can you add assertions for this in place of code comments?

[Gankra]

I did; the new debug_asserts and unwrap that imply these assumptions. I just wrote the actual assumption out to explain those asserts/unwrap.

[mrobinson]

Ah, I see now. Thanks!

[Gankra]

@glennw some high level details about your questions:

Why are some aux lists prefix and some postfix?

This can potentially be eliminated (making them all postfix), but this was done to minimally disrupt Servo and Gecko's implementations. Minimal disruption was a goal of mine to minimize the number of places I could make a mistake, since this is a fairly large change that touches a lot of different things.

Essentially, gradient stops and complex clips are pushed into the aux list before the element that uses them, via create_* APIs. So I need to be able to push that data into the display list before the actual item that uses them. This should be relatively easy for complex clips, but is harder to fix for GradientStops -- gradients are embedded in a nested type, and their presence can't be determined just by the SpecificDisplayItem.

Glyphs and filters, on the other hand, are provided at the time of pushing in the SpecificDisplayItem, and so can be stored in implicit postfix position.

This difference is also why I'm passing around _tokens. Before you knew ComplexClips were pushed in before the DisplayItem by virtue of having a ClipRegion that create_clip_region created and push_my_item consumed. Under my new design create_clip_region is push_clip_region, so there's no necessary state to pass between the two. This makes the API error-prone; it's very easy to forget to push the ClipRegion before the Item, which will break things.

So I added a zero-sized-move-only token type that push_clip_region produces and push_my_item consumes, as a way to tie the two functions together. And indeed this caught a bug that would have appeared in the gecko bindings (reusing a pushed clip between items).

Why is count and ItemRange provided?

An ItemRange is now the byte range in which a serialized slice of the given type can be found. The format of this slice is slice.len() as u64, bincode(item), bincode(item), ...

So at a basic level, the length of an ItemRange is no longer the value that the old code was trying to get, which was the number of elements. But computing the count from the length also isn't a simple matter of division for two reasons:

• Bincode provides no way to statically acquire the size of a type, because it doesn't even know. The structure of serde makes bincode fairly "dumb" in that it just gets a stream of fields when told to process a value. This is all monomorphic so it ideally optimizes well. We could guess type sizes but this would be brittle, and I'd like to avoid that. We could also serialize a value at runtime and then check its footprint, but that's... gross.

• Any type containing enums (e.g. FilterOp) has a value-dependent size. For instance Option<u64>::None occupies 4 bytes, but Option<u64>::Some occupies 12 bytes. It's genuinely impossible to derive the count from the memory footprint.

Finally the fact that slice.len() is sitting right at the front of the payload is no help to the code I threaded these values to: they don't have access to the BuiltDisplayList, and so can't read that value.

The Remaining TODOs

• length assertion: investigating if that ever got "fixed" now -- somehow trailing zeros were getting pushed into the stream!?

• push_built_display_lists: yes I intended to fill that in when I did the gecko integration work. Since pushing up this PR I have completed the implementation as part of that work, and can add it to this PR along with the rebase.

[kvark]

@gankro thanks for the wall of text the extensive description of your work! Really nice to read in the context of the PR code.

Under my new design create_clip_region is push_clip_region

This actually worries me. I'd prefer us leaving the push_* semantics only for stuff that hierarchically affects everything. For things that are basically items (e.g. push_rect, push_clip_region, etc), I'd rather have add_* prefix or something else that's less ambiguous.

[glennw]

@gankro Thanks for the explanation - that makes sense, and I definitely agree with minimal disruption to callers! Should we open an issue to consider making all those items postfix as a follow up? Would that make things more consistent in the long term?

[Gankra]

@kvark I felt push_* emphasized the transientness -- maybe set_* would be even better?

@glennw yes an issue sounds like a good idea. From my work on integrating these changes into gecko and servo, it seems like clip_region should be very easy to do, but I'm still not sure about how to do gradients well. The deserializer can do a deep match to find everything that would have gradients, but how to restructure the builder API is unclear to me. I wouldn't feel too bad if gradients were cursed to forever be prefix, since afaict they're super rare.

[Gankra]

Rebased, pushed up fixes to almost everything. Because git is complicated, some stuff ended up getting squashed into earlier commits. e.g. my changes to the examples and the impl for push_built_display_list are squashed into the relevant commits.

The "fixup impl" stuff I'll squash after review, and has most of the changes.

[Gankra]

TODO:

• investigate that assertion
• rename push_clip_region?

[Gankra]

Ok yeah that assertion is still broken. I have no idea where those trailing zeros are coming from, and it scares me, but everything seems to be working so I'm inclined to file an issue and call it a day.

[glennw]

I've run this against Servo master with the CSS and WPT tests successfully. Also did a bit of informal testing, and all seemed to be working OK.

The trailing bytes is a bit scary, but I'm OK with merging this now since it's going to bit-rot quickly otherwise.

@gankro Is this ready to merge from your perspective?

[Gankra]

Assuming y'all are fine with calling it push_clip_region

[kvark]

@gankro

Assuming y'all are fine with calling it push_clip_region

I see it as a part of a bigger (naming) problem, affecting more stuff than just clip regions. We can (and should!) address this in follow-ups.

Also, I believe that @mrobinson notes have been addressed as well. Going to merge in 20 minutes, unless there are any objections.

[mrobinson]

I think my concerns have been answered, so I'm okay with merging this. Before merging though, I have a few more style nits.

[mrobinson]

We should probably use newlines here, the single line style is not something that we typically use in WebRender as far as I know.

[mrobinson]

This should probably also be expressed as a match.

[mrobinson]

I think this could also use newlines as well.

[Gankra]

@rlhunt's nits addressed in newest commit (will squash when ready)

[Gankra]

Sorry I meant @mrobinson's comments. Also squashed.

[kvark]

Thanks @gankro ! Sounds like we are ready to take off.
@bors-servo r+

[bors-servo]

📌 Commit ff8f8db has been approved by kvark

[bors-servo]

⌛ Testing commit ff8f8db with merge 964df2f...

[bors-servo]

☀️ Test successful - status-travis
Approved by: kvark
Pushing 964df2f to master...