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

Fix font.glyph lifetimes allow proxy glyphs in cache #65

Merged
merged 1 commit into from Oct 30, 2017

Conversation

alexheretic
Copy link
Contributor

One thing I noticed about Cache::queue_glyph was that it called glyph.standalone() for each queued glyph to convert it to a 'static lifetime. I wondered why this was necessary, and when it wasn't if the performance would be improved.

Allowing the cache's queue to take glyphs with a generic lifetime is fairly simple, but I had to stare at the error messages for a while to work out why it wasn't working. There was a signature error in Font::glyph:

Before

impl<'a> Font<'a> {
    pub fn glyph<C: Into<CodepointOrGlyphId>>(&self, id: C) -> Option<Glyph> {...}
}

After

impl<'a> Font<'a> {
    pub fn glyph<C: Into<CodepointOrGlyphId>>(&self, id: C) -> Option<Glyph<'a>> {...}
}

Spot the difference? Before the signature means the Glyph takes the lifetime of the reference to the font, rather than the lifetime of the font data. This was compounded by the fact a reference to the font was used in the GlyphInner struct. This isn't necessary, as Font is a cheap-clone type anyway (a reference, or an Arc).

With this change we can see another small improvement to cache performance, in addition to the optimisation earlier today:

name                                                    control.stdout ns/iter  change.stdout ns/iter  diff ns/iter  diff %  speedup 
gpu_cache::cache_bench_tests::cache_bench_tolerance_1   3,048,049               2,878,432                  -169,617  -5.56%   x 1.06 
gpu_cache::cache_bench_tests::cache_bench_tolerance_p1  5,288,473               5,067,874                  -220,599  -4.17%   x 1.04 

Moreover, this has a larger affect downstream. In my crate gfx_glyph I'm seeing 15-24% reduction in cache interaction latency after all standalone() calls are removed (allowed by this patch).

The change from Cache -> Cache<'font> means this isn't totally backward compatible though, so should be part of the next version, ie 0.3.

Fix to glyph lifetime generation to be the font's lifetime, rather than
the reference to the font.
Remove forced .standalone() calls in cache to, allowing improved
performance for proxy glyphs.
@alexheretic
Copy link
Contributor Author

In fact if we compare the performance to the beginning of today (ie 5f8b01b), the two PRs have made a nice difference:

 name                                                    control.stdout ns/iter  change.stdout ns/iter  diff ns/iter   diff %  speedup 
 gpu_cache::cache_bench_tests::cache_bench_tolerance_1   3,306,225               2,913,608                  -392,617  -11.88%   x 1.13 
 gpu_cache::cache_bench_tests::cache_bench_tolerance_p1  6,921,851               5,130,891                -1,790,960  -25.87%   x 1.35 

@alexheretic
Copy link
Contributor Author

Let me know if you have concerns about this, because the only downside I can see is bumping the version to 0.3.

  • The Font::glyph change is backward compatible as the glyph lifetime will be greater than before.
  • Cache -> Cache<'font> is not fully back compatible, hence the version bump.
    • Current user struct fields may need to be refactored Cache<'static>.
    • Users of a Cache<'static> with a non-static Font<'a> should instead refactor to Cache<'a> for performance improvements.
    • OR keep Cache<'static> and use cache.queue_glyph(glyph.standalone()) for functionality as before.

I think for all cases refactoring is easy and performance is either improved or unaffected.

@alexheretic
Copy link
Contributor Author

I'll close this PR in a couple of days.

@jackpot51
Copy link
Member

Sorry, I missed this PR in my notifications. It looks good to me, and we can collect changes for a version 3

@jackpot51 jackpot51 merged commit a035670 into redox-os:master Oct 30, 2017
@alexheretic
Copy link
Contributor Author

@jackpot51 Thanks for taking a look. When do you think version 0.3.0 will be published?

@alexheretic alexheretic deleted the glyph-lifetime branch October 30, 2017 14:19
@jackpot51
Copy link
Member

It can be soon, to incorporate this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants