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

Add Bench mark for text render optimized TryGetGlyphMetricsAtOffset #282

Closed

Conversation

Faolan-Rad
Copy link

Description

This was me just finds Glyph Metrics when it loads and updates rather then on every TryGetGlyphMetricsAtOffset which improves performance even more

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Faolan-Rad
Copy link
Author

This is based off this pull request Speed up Unicode Data lookups and remove reflection

@JimBobSquarePants
Copy link
Member

Hi @Faolan-Rad thanks for your PR.

To accept this I would have to see two things.

  1. Proof that this area is a processing hotspot and that it requires extra optimization.
  2. Before/After benchmark results showing the improvement from the change.

I would need some convincing to see the usefulness of this change. GlyphPositioningCollection.TryGetGlyphMetricsAtOffset(...) is only called during line breaking which involves a sequential run against the codepoints within the string. This means that a given offset is only requested one time which means any cache lookup will always return false.

@JimBobSquarePants JimBobSquarePants deleted the branch SixLabors:js/perf-tweaks June 29, 2022 09:13
@JimBobSquarePants
Copy link
Member

Sorry @Faolan-Rad This was auto closed when I merged the base branch. If you still feel like it is required please repoint the PR at the main branch.

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

Successfully merging this pull request may close these issues.

3 participants