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

Update cosmic-text and resvg #2416

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update cosmic-text and resvg #2416

wants to merge 2 commits into from

Conversation

hecrj
Copy link
Member

@hecrj hecrj commented May 1, 2024

This PR updates cosmic-text to 0.11 and resvg to 0.41.

It seems there may be a bit of a performance regression with this update—I am noticing a ~5% increase in render time in most of the examples.

I created a new "layered text" benchmark and it caught a 2% regression:

image

The benchmark tests static text, so I suspect the regression caught here may be caused by the hashing of CacheKey—which now includes an additional CacheKeyFlags field.

I suspect regressions here are normal since these libraries are still evolving, getting more complex, and satisfying more use cases. In any case, I figured I'd share my results just in case someone may feel like investigating further.

Also important to mention that I originally updated my glyphon fork to catch up to the main branch upstream, but apparently that introduces yet another regression—a bigger one!

image

I suspect the main culprit here is the additional bind group changes introduced by grovesNL/glyphon#88, specially when using multiple TextRenderer instances (precisely the scenario of the benchmark!).

@hecrj hecrj added improvement An internal improvement text change labels May 1, 2024
@hecrj hecrj added this to the 0.13 milestone May 1, 2024
@hecrj hecrj mentioned this pull request May 1, 2024
@wyatt-herkamp
Copy link
Contributor

wyatt-herkamp commented May 2, 2024

I suspect the main culprit here is the additional bind group changes introduced by grovesNL/glyphon#88, specially when using multiple TextRenderer instances (precisely the scenario of the benchmark!).

Due to the size of glyphon and the added complexity of updating WGPU and Cosmic Text. I was thinking the other day. That Iced should just implement the Text rendering directly in Iced. Glyphon is not a huge amount of code and wouldn't be that hard.

It would give us more control and make it easier to update when WGPU or Cosmic Text updates.

And with recent security issues coming up with supply chain attacks. Shrinking the dependency tree can be a good thing.

@grovesNL
Copy link

grovesNL commented May 3, 2024

@wyatt-herkamp I totally understand that it might be useful to avoid lag in updating wgpu/cosmic-text and avoid a possible area for supply chain attacks by handling the text rendering in iced directly.

I'd just ask to consider the positive network effects of code sharing the text renderer in addition to those update/supply chain points. For example, people using glyphon but don't contribute to iced have updated dependencies, improved performance, fixed bugs, added new functionality that could benefit iced in the future, etc.

For context, I created glyphon to use in a text-heavy visualization for a commercial application, so I'm committed to keeping it updated and performant while still being flexible about new use cases (e.g., specializing certain use cases where it makes sense).

@hecrj
Copy link
Member Author

hecrj commented May 8, 2024

@grovesNL I have opened a couple of PRs in glyphon that fix the regression:

@hecrj
Copy link
Member Author

hecrj commented May 8, 2024

glyphon regression is fixed with the latest changes 🎉

image

master is still a bit faster, but that's probably because of cosmic-text's layout improvements:

image

We need a benchmark with actual dynamic text to properly measure the difference.

@hecrj
Copy link
Member Author

hecrj commented May 8, 2024

I just created a dynamic text benchmark in master:

image

Small regression only noticeable with important workloads.

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

Successfully merging this pull request may close these issues.

None yet

3 participants