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

Sourcemap performance improvements #668

Merged
merged 3 commits into from
May 6, 2023
Merged

Sourcemap performance improvements #668

merged 3 commits into from
May 6, 2023

Conversation

filiptibell
Copy link
Contributor

This PR brings two performance improvements to the rojo sourcemap command:

  • Use rayon with a small threadpool to parallelize sourcemap generation while still keeping startup cost very low
  • Remove conversions to owned strings and use lifetimes tied to the dom instead, which mostly improves performance with the --include-non-scripts flag enabled

From my personal testing on an M1 mac this decreases the sourcemap generation time of our games by 2x or more, from ~20ms to ~8ms on one project and ~30ms to ~15ms on another. Generation is pretty fast to begin with but since sourcemaps are heavily used in interactive tools (like luau-lsp) a difference of a couple frames can be great for ux.

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a glance, that clippy CI failure seems unrelated to this (which is worrying in its own right but that's a different issue). While I'm not sure what impact pulling rayon as a dependency will have on build time or executable size, I'm guessing it will be insignificant so that's whatever.

Can you add this to the Changelog? After that, I'm not seeing a reason this couldn't be merged, though this is my first time reviewing a change to Rojo proper.

@filiptibell
Copy link
Contributor Author

Can you add this to the Changelog?

Sure thing, I tried to find a similar previous note in the changelog and #548 was the most similar so its more or less copy pasted from that.

I'm not sure what impact pulling rayon as a dependency will have on build time or executable size

It's a pretty lightweight dependency afaik but pulls in some heavier things such as crossbeam. Lock file only shows rayon lib itself as a new dependency though, so seems we already depend on the rest.


I also went ahead and used num_cpus to cap the number of threads we create in the threadpool based on available cpu count so we don't create unnecessary threads for low-powered systems. This was another transitive dependency we already used so it won't increase build time or binary size.

Copy link
Member

@Corecii Corecii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Since this is my first approving review as a maintainer I'd like additional input from other common contributors. We already have Dekkonot's approval here. @Kampfkarren think this is good to merge?

@Kampfkarren Kampfkarren enabled auto-merge (squash) May 6, 2023 08:00
@Kampfkarren Kampfkarren disabled auto-merge May 6, 2023 08:00
@Kampfkarren Kampfkarren merged commit 305423b into rojo-rbx:master May 6, 2023
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
This PR brings two performance improvements to the `rojo sourcemap`
command:

- Use `rayon` with a small threadpool to parallelize sourcemap
generation while still keeping startup cost very low
- Remove conversions to owned strings and use lifetimes tied to the dom
instead, which mostly improves performance with the
`--include-non-scripts` flag enabled

From my personal testing on an M1 mac this decreases the sourcemap
generation time of our games by 2x or more, from ~20ms to ~8ms on one
project and ~30ms to ~15ms on another. Generation is pretty fast to
begin with but since sourcemaps are heavily used in interactive tools
(like luau-lsp) a difference of a couple frames can be great for ux.
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.

4 participants