iced-rs / iced

A cross-platform GUI library for Rust, inspired by Elm
https://iced.rs
MIT License
22.96k stars 1.06k forks source link

Update `cosmic-text` and `resvg` #2416

Open hecrj opened 2 weeks ago

hecrj commented 2 weeks ago

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 https://github.com/grovesNL/glyphon/pull/88, specially when using multiple TextRenderer instances (precisely the scenario of the benchmark!).

wyatt-herkamp commented 2 weeks ago

I suspect the main culprit here is the additional bind group changes introduced by https://github.com/grovesNL/glyphon/pull/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 commented 2 weeks ago

@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 commented 1 week ago

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

hecrj commented 1 week ago

glyphon regression is fixed with the latest changes :tada:

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 commented 1 week ago

I just created a dynamic text benchmark in master:

image

Small regression only noticeable with important workloads.