Glyph key scratch store

[djg]

Implement scratch storage of GlyphKeys for #3324

---

This change is 

[djg]

Will post Gecko try run once I get gecko compiling with the new webrender.

[kvark]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @djg)

---

webrender/src/storage.rs, line 8 at r1 (raw file):

#[macro_export]
macro_rules! storage_index_impl {

I would implement this differently:

struct Index<T>(u32, PhantomData<T>);

struct Storage<T> { ... }

impl<T> Storage<T> {
    pub fn push(&mut self, t: T) -> Index<T> {
        ...
    }
    ...
}

I think it would be better, because:

• no macro magic
• you can clearly see what an index type is for
• slightly less code
• no need for index types to expose Into and From to the outside world

---

webrender/src/storage.rs, line 33 at r1 (raw file):

#[derive(Debug, Copy, Clone)]
pub struct Range<I> {

can we use std::ops::Range instead?

---

webrender/src/storage.rs, line 38 at r1 (raw file):

}

impl<I: From<usize>> Default for Range<I> {

where is this used?

---

webrender/src/storage.rs, line 47 at r1 (raw file):

}

impl<I: Into<usize> + PartialOrd> Range<I> {

Into<usize> bound doesn't appear to be needed here

---

webrender/src/storage.rs, line 58 at r1 (raw file):

}

impl<T, I: Into<usize> + From<usize>> Storage<T, I> {

Into<usize> doesn't appear to be needed here

---

webrender/src/storage.rs, line 77 at r1 (raw file):

}

impl<T, I: From<usize> + Into<usize>> Index<I> for Storage<T, I> {

here and below we don't need From<usize> bound

[djg]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kvark)

---

webrender/src/storage.rs, line 8 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I would implement this differently:

struct Index<T>(u32, PhantomData<T>);

struct Storage<T> { ... }

impl<T> Storage<T> {
    pub fn push(&mut self, t: T) -> Index<T> {
        ...
    }
    ...
}

I think it would be better, because:

• no macro magic
• you can clearly see what an index type is for
• slightly less code
• no need for index types to expose Into and From to the outside world

Having Index types expose Into and From makes them less "safe", so I'm going to accept this and refactor the Index to be a generic. Having PhantomData<T> makes automatic derivation of Copy, PartialEq, and PartialOrd a bit more tricky.

---

webrender/src/storage.rs, line 33 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we use std::ops::Range instead?

It is possible but it's a bit less ergonomic.

I wanted to use Range::is_empty() but it's an experimental feature.
Also, std::ops::Range doesn't implement Copy (rust-lang/rust#27186 (comment)) which means that Index<Range<T>> for Storage<T> is a bit ugly to use. I was attempting to get slice-like syntax for obtaining a range of items from Storage<T>.

---

webrender/src/storage.rs, line 38 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

where is this used?

This is used in the next patch when creating PrimitiveKeyKind::TextRun.

let run_index = prim_store.text_runs.push(TextRunPrimitive {
    used_font: font.clone(),
    glyph_keys_range: Range::default(),
    shadow,
});

---

webrender/src/storage.rs, line 47 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Into<usize> bound doesn't appear to be needed here

Done.

---

webrender/src/storage.rs, line 58 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Into<usize> doesn't appear to be needed here

Done.

---

webrender/src/storage.rs, line 77 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

here and below we don't need From<usize> bound

Done.

[djg]

Try run result for first version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a1353c60b3cedfea396330beab65b825170b821&selectedJob=212544428

[gw3583]

It'd be interesting to get some separate profiles of the change to use a scratch buffer vs. using the chunked storage. It may well be that a normal Vec with a reasonable reserve estimate up front is actually faster than the chunked storage?

Running benchmarks/text-rendering.yaml is a reasonable stress test of drawing a lot of text that is easy to profile within wrench. You'd need to run with -r though to replay the display list each time you hit right-arrow.

[kvark]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @djg)

---

webrender/src/storage.rs, line 33 at r1 (raw file):

Previously, djg (Dan Glastonbury) wrote…

It is possible but it's a bit less ergonomic.

I wanted to use Range::is_empty() but it's an experimental feature.
Also, std::ops::Range doesn't implement Copy (rust-lang/rust#27186 (comment)) which means that Index<Range<T>> for Storage<T> is a bit ugly to use. I was attempting to get slice-like syntax for obtaining a range of items from Storage<T>.

where is is_empty() used?
As for Index<Range<T>> I agree that it's unfortunate that it would require Clone. However, we can implement it as Index<&'a Range<T>> and that would be fairly concise

[djg]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kvark)

---

webrender/src/storage.rs, line 33 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

where is is_empty() used?
As for Index<Range<T>> I agree that it's unfortunate that it would require Clone. However, we can implement it as Index<&'a Range<T>> and that would be fairly concise

is_empty is used at prim_store.rs.1360. e2149c5#diff-33603c013bcfbe19d5d30c93a810759aR1360

[kvark]

@djg try looks good, and my concerns are addressed (for the most part). Could you check that benchmark mentioned by @gw3583 before we go?

[djg]

I'll run the benchmark that @gw3583 mentioned once I get my current interning work to a good point.

@gw3583, I was thinking that sizing the initial storage to ~64KB

[gw3583]

@bors-servo r+

[bors-servo]

📌 Commit 3531030 has been approved by gw3583

[bors-servo]

⌛ Testing commit 3531030 with merge a749b63...

[gw3583]

(Discussed on IRC - I was initially misreading how the storage type works. This should be a simple win, and we'll extend it with recycle functionality as a follow up).

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing a749b63 to master...