Recycle Core Graphics contexts when rasterizing glyphs. #3071

pcwalton · 2018-09-17T22:34:28Z

This should avoid locks and save some memory allocation time.

Closes #2406.

r? @gw3583 (or whoever wants to)

This change is

gw3583 · 2018-09-18T00:32:07Z

Looks like some compile errors on CI:

error[E0425]: cannot find value `kCGImageAlphaPremultipliedFirst` in this scope
   --> webrender/src/platform/macos/font.rs:822:34
    |
822 |             GlyphType::Bitmap => kCGImageAlphaPremultipliedFirst,
    |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in this scope
help: possible candidate is found in another module, you can import it into scope
    |
5   | use core_graphics::base::kCGImageAlphaPremultipliedFirst;
    |
error: unused import: `CG_AFFINE_TRANSFORM_IDENTITY`
  --> webrender/src/platform/macos/font.rs:22:31
   |
22 | use core_graphics::geometry::{CG_AFFINE_TRANSFORM_IDENTITY, CGAffineTransform, CGPoint, CGSize};
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

@lsalzman would probably be the best person to look over this.

lsalzman · 2018-09-18T02:24:35Z

lgtm

kvark · 2018-09-18T14:00:03Z

Annoying unused variable/method warnings=errors:

error: field is never used: `graphics_context`
  --> webrender/src/platform/macos/font.rs:43:5
   |
43 |     graphics_context: GraphicsContext,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D dead-code` implied by `-D warnings`

error: field is never used: `vector_context`
   --> webrender/src/platform/macos/font.rs:739:5
    |
739 |     vector_context: CGContext,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^

error: field is never used: `vector_context_size`
   --> webrender/src/platform/macos/font.rs:740:5
    |
740 |     vector_context_size: Size2D<u32>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: field is never used: `bitmap_context`
   --> webrender/src/platform/macos/font.rs:741:5
    |
741 |     bitmap_context: CGContext,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^

error: field is never used: `bitmap_context_size`
   --> webrender/src/platform/macos/font.rs:742:5
    |
742 |     bitmap_context_size: Size2D<u32>,
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: method is never used: `get_context`
   --> webrender/src/platform/macos/font.rs:756:5
    |
756 | /     fn get_context(&mut self, size: &Size2D<u32>, glyph_type: GlyphType)
757 | |                    -> &mut CGContext {
758 | |         let (cached_context, cached_size) = match glyph_type {
759 | |             GlyphType::Vector => {
...   |
773 | |         cached_context
774 | |     }
    | |_____^

error: method is never used: `get_rasterized_pixels`
   --> webrender/src/platform/macos/font.rs:776:5
    |
776 | /     fn get_rasterized_pixels(&mut self, size: &Size2D<u32>, glyph_type: GlyphType)
777 | |                              -> Vec<u8> {
778 | |         let (cached_context, cached_size) = match glyph_type {
779 | |             GlyphType::Vector => (&mut self.vector_context, &self.vector_context_size),
...   |
793 | |         result
794 | |     }
    | |_____^

pcwalton · 2018-09-18T22:16:31Z

Now blocked on servo/core-foundation-rs#255.

pcwalton · 2018-09-19T18:28:13Z

Unblocked now.

kvark · 2018-09-19T18:48:06Z

webrender/Cargo.toml

@@ -73,5 +73,5 @@ dwrote = "0.4.1"

 [target.'cfg(target_os = "macos")'.dependencies]
 core-foundation = "0.6"
-core-graphics = "0.16"
-core-text = { version = "11", default-features = false }
+core-graphics = "^0.17.1"


why having ^ here?

I wasn't sure what the default was when all subversions are specified, but I see that it's ^ by default, so I'll remove it.

pcwalton · 2018-09-19T20:36:44Z

@bors-servo: r=kvark

bors-servo · 2018-09-19T20:36:45Z

📌 Commit 21c1c79 has been approved by kvark


        Auto merge of #3071 - pcwalton:cg-context-recycling, r=kvark

@gw3583

Recycle Core Graphics contexts when rasterizing glyphs. This should avoid locks and save some memory allocation time. Closes #2406. r? @gw3583 (or whoever wants to)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3071)

bors-servo · 2018-09-19T22:56:43Z

⌛ Testing commit 21c1c79 with merge 16cc60b...

bors-servo · 2018-09-20T00:56:30Z

💔 Test failed - status-taskcluster

jrmuizel · 2018-09-20T03:09:42Z

     Running `sccache rustc --crate-name gleam /Users/tcworker/.cargo/registry/src/github.com-1ecc6299db9ec823/gleam-0.6.0/src/lib.rs --crate-type lib --emit=dep-info,metadata -C debuginfo=2 -C metadata=da18e35026e3e870 -C extra-filename=-da18e35026e3e870 --out-dir /Users/tcworker/webrender/target/debug/deps -L dependency=/Users/tcworker/webrender/target/debug/deps --cap-lints allow --deny warnings`
     Running `sccache rustc --crate-name webrender webrender/src/lib.rs --crate-type lib --emit=dep-info,metadata -C debuginfo=2 --cfg 'feature="pathfinder"' --cfg 'feature="pathfinder_font_renderer"' --cfg 'feature="pathfinder_gfx_utils"' --cfg 'feature="pathfinder_partitioner"' --cfg 'feature="pathfinder_path_utils"' -C metadata=0fe980b66f1f124c -C extra-filename=-0fe980b66f1f124c --out-dir /Users/tcworker/webrender/target/debug/deps -C incremental=/Users/tcworker/webrender/target/debug/incremental -L dependency=/Users/tcworker/webrender/target/debug/deps --extern thread_profiler=/Users/tcworker/webrender/target/debug/deps/libthread_profiler-210b43b04f328727.rmeta --extern pathfinder_font_renderer=/Users/tcworker/webrender/target/debug/deps/libpathfinder_font_renderer-aec50552663d09e8.rmeta --extern byteorder=/Users/tcworker/webrender/target/debug/deps/libbyteorder-d8ff85130b9effdd.rmeta --extern app_units=/Users/tcworker/webrender/target/debug/deps/libapp_units-427bcd39dcf04a07.rmeta --extern bincode=/Users/tcworker/webrender/target/debug/deps/libbincode-86fd0213dc393d0e.rmeta --extern num_traits=/Users/tcworker/webrender/target/debug/deps/libnum_traits-9319f52eb37cdcfd.rmeta --extern rayon=/Users/tcworker/webrender/target/debug/deps/librayon-ca8a23f4e4d5055f.rmeta --extern core_foundation=/Users/tcworker/webrender/target/debug/deps/libcore_foundation-50d98b12587deed2.rmeta --extern log=/Users/tcworker/webrender/target/debug/deps/liblog-0aeb91441181c1b3.rmeta --extern bitflags=/Users/tcworker/webrender/target/debug/deps/libbitflags-84c15de524502337.rmeta --extern core_text=/Users/tcworker/webrender/target/debug/deps/libcore_text-c82053a88dd9b0ec.rmeta --extern cfg_if=/Users/tcworker/webrender/target/debug/deps/libcfg_if-9905ff195a0231b6.rmeta --extern core_graphics=/Users/tcworker/webrender/target/debug/deps/libcore_graphics-cff5fccd4be3442a.rmeta --extern pathfinder_gfx_utils=/Users/tcworker/webrender/target/debug/deps/libpathfinder_gfx_utils-34ccf080a17bb4e3.rmeta --extern gleam=/Users/tcworker/webrender/target/debug/deps/libgleam-da18e35026e3e870.rmeta --extern pathfinder_partitioner=/Users/tcworker/webrender/target/debug/deps/libpathfinder_partitioner-647f796f51448845.rmeta --extern time=/Users/tcworker/webrender/target/debug/deps/libtime-d07a0d86cde22262.rmeta --extern webrender_api=/Users/tcworker/webrender/target/debug/deps/libwebrender_api-f3ae00872a6e1495.rmeta --extern fxhash=/Users/tcworker/webrender/target/debug/deps/libfxhash-6c5587b4dbc81d42.rmeta --extern plane_split=/Users/tcworker/webrender/target/debug/deps/libplane_split-319e513e8eebae50.rmeta --extern smallvec=/Users/tcworker/webrender/target/debug/deps/libsmallvec-8052fc8892857286.rmeta --extern pathfinder_path_utils=/Users/tcworker/webrender/target/debug/deps/libpathfinder_path_utils-1036b596e0aa77c0.rmeta --extern lazy_static=/Users/tcworker/webrender/target/debug/deps/liblazy_static-27ae427397fdecc2.rmeta --extern euclid=/Users/tcworker/webrender/target/debug/deps/libeuclid-2118c06bd0cf44e6.rmeta --deny warnings`
error[E0277]: the trait bound `core_graphics::font::CGFont: std::convert::From<glyph_rasterizer::pathfinder::NativeFontHandleWrapper<'_>>` is not satisfied
  --> webrender/src/glyph_rasterizer/pathfinder.rs:49:27
   |
49 |                 drop(self.add_native_font(&font_key, NativeFontHandleWrapper(native_font_handle)))
   |                           ^^^^^^^^^^^^^^^ the trait `std::convert::From<glyph_rasterizer::pathfinder::NativeFontHandleWrapper<'_>>` is not implemented for `core_graphics::font::CGFont`
   |


        Recycle Core Graphics contexts when rasterizing glyphs.

This should avoid locks and save some memory allocation time. This patch also updates Pathfinder so we can keep the `core-graphics` crate versions in sync. Closes #2406.

pcwalton · 2018-09-20T04:02:06Z

So the problem here is that we had two versions of core-graphics in use, one by pathfinder_font_renderer and one by WebRender, and they clashed. Ordinarily, the solution would be straightforward: update Pathfinder to use the newest version of core-graphics. But pathfinder_font_renderer no longer exists, because it's been replaced with font-kit. We've had an understandable reluctance to take a dependency on font-kit while we're in ship mode. So I decided to make a webrender branch of Pathfinder from the latest version before the font-kit changeover and update the core-graphics dependency in that branch.

I'd like to get review on this change to make sure everyone is OK with it, so:

r? @gw3583

kvark · 2018-09-20T14:37:13Z

Want to make a new try push to be safe?

pcwalton · 2018-09-20T16:21:45Z

I can't imagine this would affect anything on try, because only the Pathfinder code changed.

pcwalton · 2018-09-20T16:22:12Z

@bors-servo: r=kvark

bors-servo · 2018-09-20T16:22:13Z

📌 Commit 07f1422 has been approved by kvark

bors-servo · 2018-09-20T16:22:17Z

⌛ Testing commit 07f1422 with merge a601f9c...


        Auto merge of #3071 - pcwalton:cg-context-recycling, r=kvark

@gw3583

Recycle Core Graphics contexts when rasterizing glyphs. This should avoid locks and save some memory allocation time. Closes #2406. r? @gw3583 (or whoever wants to)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3071)

bors-servo · 2018-09-20T17:43:41Z

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

kvark · 2018-09-20T17:55:34Z

Well, the core-graphics dependency changed, that's what I was concerned about.

pcwalton force-pushed the pcwalton:cg-context-recycling branch from 22d49b6 to 68cc5c2 Sep 18, 2018

pcwalton force-pushed the pcwalton:cg-context-recycling branch from 68cc5c2 to c79ab84 Sep 18, 2018

pcwalton force-pushed the pcwalton:cg-context-recycling branch from c79ab84 to c40f620 Sep 19, 2018

kvark approved these changes Sep 19, 2018

View changes

pcwalton force-pushed the pcwalton:cg-context-recycling branch from c40f620 to 21c1c79 Sep 19, 2018

pcwalton force-pushed the pcwalton:cg-context-recycling branch from 21c1c79 to 04b66c8 Sep 20, 2018

Recycle Core Graphics contexts when rasterizing glyphs.

Loading status checks…

07f1422

This should avoid locks and save some memory allocation time. This patch also updates Pathfinder so we can keep the `core-graphics` crate versions in sync. Closes #2406.

pcwalton force-pushed the pcwalton:cg-context-recycling branch from 04b66c8 to 07f1422 Sep 20, 2018

kvark approved these changes Sep 20, 2018

View changes

bors-servo merged commit 07f1422 into servo:master Sep 20, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

bors-servo mentioned this pull request Sep 20, 2018

Construct split planes with help of the inversed transform #3093

Merged

servo / webrender

Recycle Core Graphics contexts when rasterizing glyphs. #3071

Recycle Core Graphics contexts when rasterizing glyphs. #3071

pcwalton commented Sep 17, 2018 •

edited by larsbergstrom

gw3583 commented Sep 18, 2018

lsalzman commented Sep 18, 2018

kvark commented Sep 18, 2018

pcwalton commented Sep 18, 2018

pcwalton commented Sep 19, 2018

This comment has been minimized.

This comment has been minimized.

pcwalton commented Sep 19, 2018

bors-servo commented Sep 19, 2018

bors-servo commented Sep 19, 2018

bors-servo commented Sep 20, 2018

jrmuizel commented Sep 20, 2018

pcwalton commented Sep 20, 2018 •

edited

kvark commented Sep 20, 2018

pcwalton commented Sep 20, 2018

pcwalton commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

kvark commented Sep 20, 2018

servo / webrender

Join GitHub today

Recycle Core Graphics contexts when rasterizing glyphs. #3071

Recycle Core Graphics contexts when rasterizing glyphs. #3071

Conversation

pcwalton commented Sep 17, 2018 • edited by larsbergstrom

gw3583 commented Sep 18, 2018

lsalzman commented Sep 18, 2018

kvark commented Sep 18, 2018

pcwalton commented Sep 18, 2018

pcwalton commented Sep 19, 2018

This comment has been minimized.

kvark Sep 19, 2018

This comment has been minimized.

pcwalton Sep 19, 2018

pcwalton commented Sep 19, 2018

bors-servo commented Sep 19, 2018

bors-servo commented Sep 19, 2018

bors-servo commented Sep 20, 2018

jrmuizel commented Sep 20, 2018

pcwalton commented Sep 20, 2018 • edited

kvark commented Sep 20, 2018

pcwalton commented Sep 20, 2018

pcwalton commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

bors-servo commented Sep 20, 2018

kvark commented Sep 20, 2018

pcwalton commented Sep 17, 2018 •

edited by larsbergstrom

pcwalton commented Sep 20, 2018 •

edited