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

Plugin/skia-canvas memory leak issue #18

Closed
mpaperno opened this issue Aug 15, 2023 · 2 comments
Closed

Plugin/skia-canvas memory leak issue #18

mpaperno opened this issue Aug 15, 2023 · 2 comments

Comments

@mpaperno
Copy link
Collaborator

mpaperno commented Aug 15, 2023

As discussed on Discord, a memory leak has been found in DI plugin version 1.1+ and tracked down to the latest version of skia-canvas (SC) library which the plugin uses for drawing. An issue has been filed there with details.

The leak affects only SC v1.0.1, but v1.0.0 had a bug which would crash the plugin on systems w/out GPU rendering support. (Ironically, it seems the commit which fixed that bug also introduced the memory leak.)

SC v1.0 introduced a GPU rendering option, roundRect() drawing method which we use, and some other improvements (change log).

The current workaround is to force GPU rendering to avoid the memory leak. I've already removed the async SC calls which were part of the problem, and that's not a big deal at all (those calls are already wrapped in async methods anyway). But disabling GPU will cause the leak again.

EDIT: The only way to avoid the leak, it seems, to use the synchronous SC export functions (and force GPU rendering), but this has a significant performance impact on the plugin when generating multiple images at the same time.

So we have a bit of a limbo situation since we're dependent on SC (or some drawing lib, anyway).

Here are the options I can think of at this point:

  1. Wait for SC dev or a fork to fix the issue (could be a long wait, the project doesn't seem very active and there are stale issues and PRs in the repo).
  2. Try to fix the issue ourselves (personally I have no experience with Rust or interfacing with Node, and not top on my list of things to learn).
  3. Stay on SC v1.0.1 and force sync output and GPU rendering (until 1. or 2. happen or hell freezes over :) - temporarily remove the GPU render plugin setting and per-icon option.
  4. Revert to SC < 1.0 - Lose GPU rendering capability (and again with the setting/option), and lose roundRect() which I'd need to write (not a huge deal, already have some similar code elsewhere I can translate, or find a "polyfill" online). Possibly some "regression" issues due to fixes in SC v1.0, but probably nothing major.
  5. Find another drawing library -- I've tried node-canvas (missing lots of features, including filters, good text drawing, Path2D, etc) and canvas which is also Skia-based and seemed promising until it had much worse memory leak than SC. Neither of those use GPU acceleration anyway, BTW. There are a couple other smaller projects but none of them come close to what we need... I couldn't find anything else that seemed worth trying.
  6. Rewrite the plugin in some other language. This still requires a graphics drawing library of course. All graphics libraries are written in C/C++ so that would be the obvious choice to me :)

I'm not sure GPU rendering is all that important, though it's nice to have the option to offload some of the work. OTOH I didn't like forcing use of the GPU, which is why I had added that as a setting in the first place. Given all that, right now I'm very slightly leaning towards option 4.

Input appreciated! Thanks,
-Max

@mpaperno
Copy link
Collaborator Author

I take back what I said about using SC synchronous export functions not being a big deal. Turns out there's a significant difference when generating multiple images at the same time. Looking closer at the SC code, the async methods launch an actual new system thread (in Rust).

Given that, seems like option 3 is out of the running. Or live with the slow leak... it's pretty minor until you run continuous icon generation for hours.

@mpaperno
Copy link
Collaborator Author

Turned out, to top things off, that skia-canvas < 1.0.0 had significantly worse overall rendering performance, at least during simultaneous operations. Even with GPU disabled on v1+, it's still much faster (I don't actually see a performance difference between CPU and GPU here).

So yea, went with option 2... now I have a fork, yay. Actually took longer to figure out how to publish all the binaries to automate npm builds. All for this, basically.

Anyway, hope that works... awaiting further testing results.

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

No branches or pull requests

1 participant