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

Unable to parallelize pixmap loading #67

Open
nyurik opened this issue Oct 12, 2023 · 4 comments
Open

Unable to parallelize pixmap loading #67

nyurik opened this issue Oct 12, 2023 · 4 comments

Comments

@nyurik
Copy link
Contributor

nyurik commented Oct 12, 2023

with the recent release, it is no longer possible to pre-process each SVG pixmap independently, and merge the results. I was doing it all in parallel using Tokio, and I think there is a significant enough benefit, esp for a high-load scenario, to offer rapid sprite generation.

How could we make this a bit more optimized? Thx!

@flother
Copy link
Owner

flother commented Oct 12, 2023

Did you run in to the same problem I did in b94d095, that resvg::usvg::Tree isn't thread-safe?

@nyurik
Copy link
Contributor Author

nyurik commented Oct 12, 2023

I am not sure what you mean. From the looks of it, Tree and its dependencies mostly use safe-only code, without native deps. What issue did you run into?

@flother
Copy link
Owner

flother commented Oct 13, 2023

Before b94d095 I was using Rayon to parallelise loading SVGs. But when I switched from using BTreeMap<String, Pixmap> to BTreeMap<String, sprite::Sprite> the compiler complained that:

Rc<RefCell<rctree::NodeData<NodeKind>>> cannot be sent between threads safely

Seeing as using Rayon was really just a silly case of premature optimisation, I removed it without looking into the underlying problem.

What problem are you seeing?

@nyurik
Copy link
Contributor Author

nyurik commented Oct 13, 2023

I am not seeing any problems, other than the desire to optimize the process (in theory we should have proper benchmarks for all this). Rasterisation (getting pixmap) sounds like an expensive process, so if it can be tied to IO, I think it would speed things up. I was not using Rayon. I use async/tokio.

See https://github.com/maplibre/martin/blob/99b700b1e960c54c8abe1058c622acd7441a1cae/martin/src/sprites/mod.rs#L153

The main idea: create multiple futures, each of which will read the file and parse it. Await all futures together - so once they are all done, pass it on to Spreet to generate the spritesheet (this is what i had in the previous version). Now I do almost the same, except that I only await file loading, whereas pixmap is generated sequentially by Spreet. Notice that there are no Rc or RefCell or any other things like that.

My thinking is that we could have two Sprite types - loaded and parsed (this can be done using this amazing pattern), so a user can pre-render the data async if needed, or let Spreet do the rendering.

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

2 participants