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

perf: Performance improvements #4959

Merged
merged 14 commits into from
Aug 22, 2023
Merged

perf: Performance improvements #4959

merged 14 commits into from
Aug 22, 2023

Conversation

TimBeyer
Copy link
Contributor

@TimBeyer TimBeyer commented Aug 18, 2023

What this PR does / why we need it:

This PR adds a bunch of performance improvements, and replaces Bluebird with native promises.

Bluebird

Let's start off with Bluebird: The bluebird repo itself recommends moving to native promises and bluebird is no longer seeing any changes. Meanwhile native promises have seen big performance improvements and also usually show up better in stack traces and in performance profiles.

Repo scan tree indexing

The main performance improvement comes for GARDEN_GIT_SCAN_MODE=repo.
It now indexes the list of files into a tree structure for faster access yielding large improvements in graph resolution speeds for large projects.

Here's a comparison between the current main version and the updated one, run on an M2 mac on a repo of 11032 files with 500 modules.

Using main:

time GARDEN_GIT_SCAN_MODE=repo gdev validate
Validate ✔️

Project is configured with `apiVersion: garden.io/v0`, running with backwards compatibility.
ℹ garden               → Running in Garden environment dev.default
Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
ℹ providers            → Getting status...
✔ providers            → Cached (took 2.1 sec)
ℹ providers            → Run with --force-refresh to force a refresh of provider statuses.
ℹ graph                → Resolving actions and modules...
ℹ graph                → Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
✔ graph                → Done (took 16.5 sec)

OK ✔️
GARDEN_GIT_SCAN_MODE=repo ~/Development/garden/garden/bin/garden validate  76.32s user 7.61s system 133% cpu 1:02.88 total

Using this PR:

time GARDEN_GIT_SCAN_MODE=repo gdev validate
Validate ✔️

Project is configured with `apiVersion: garden.io/v0`, running with backwards compatibility.
ℹ garden               → Running in Garden environment dev.default
Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
ℹ providers            → Getting status...
✔ providers            → Cached (took 2.1 sec)
ℹ providers            → Run with --force-refresh to force a refresh of provider statuses.
ℹ graph                → Resolving actions and modules...
ℹ graph                → Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
✔ graph                → Done (took 8 sec)

OK ✔️
GARDEN_GIT_SCAN_MODE=repo ~/Development/garden/garden/bin/garden validate  64.68s user 6.62s system 136% cpu 52.410 total

As you can see the overall performance improvements are still moderate, but the graph resolution is almost 2x faster.
This makes the repo scan mode the most performant option especially when running in our single binary build which seems to have a lot of overhead for the default scan mode.

For comparison here are both scan modes with our current 0.13.12 binary version on the same repo:

time GARDEN_GIT_SCAN_MODE=repo garden validate
Validate ✔️

Project is configured with `apiVersion: garden.io/v0`, running with backwards compatibility.
ℹ garden               → Running in Garden environment dev.default
Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
ℹ providers            → Getting status...
✔ providers            → Cached (took 2.2 sec)
ℹ providers            → Run with --force-refresh to force a refresh of provider statuses.
ℹ graph                → Resolving actions and modules...
ℹ graph                → Scanning repository at /Users/tim/Development/garden/garden-large-repo-generator
✔ graph                → Done (took 15.9 sec)

OK ✔️
GARDEN_GIT_SCAN_MODE=repo garden validate  78.09s user 8.10s system 138% cpu 1:02.22 total
time garden validate
Validate ✔️

Project is configured with `apiVersion: garden.io/v0`, running with backwards compatibility.
ℹ garden               → Running in Garden environment dev.default
ℹ providers            → Getting status...
✔ providers            → Cached (took 2.1 sec)
ℹ providers            → Run with --force-refresh to force a refresh of provider statuses.
ℹ graph                → Resolving actions and modules...
✔ graph                → Done (took 26.2 sec)

OK ✔️
garden validate  88.09s user 28.67s system 160% cpu 1:12.63 total

You can see that the repo scan mode shows identical performance to the version running just on node for development.
Meanwhile graph resolution on the default scan mode with the binary takes 26 seconds to resolve the graph while just on node it's around 15 seconds (not in the outputs above since I didn't want to add even more noisy text here).

So the new implementation with repo mode should outperform the default scan mode in the binary by a good 3x still when it comes to graph resolution.

GC Improvements

We increase max-semi-space to 64M which should lead to less GC pauses and better performance at the cost of slightly higher memory consumption.

See https://github.com/nodejs/node/blob/main/doc/api/cli.md#useful-v8-options
Also see https://www.alibabacloud.com/blog/better-node-application-performance-through-gc-optimization_595119 and nodejs/node#42511 for some details on impact.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@TimBeyer TimBeyer requested review from edvald and thsig August 18, 2023 09:56
@TimBeyer TimBeyer marked this pull request as ready for review August 18, 2023 09:57
@TimBeyer TimBeyer requested a review from a team August 18, 2023 10:47

const relativePath = file.path.slice(this.ownPath.length)
// We use absolute paths so the first part of the split is always an empty string
const [_, subpathSegment, nextSegment] = relativePath.split(path.sep)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I wasn't sure if we normalize everything to POSIX paths also for glob compatibility, or if we do need to use path.sep so that windows paths get split correctly as well.

That probably would also impact the .startsWith part above.

@vvagaytsev
Copy link
Collaborator

Wow! This is super awesome! 🚀 Huge thanks for such a great job! ✨
I need more time to take a closer look in review it in detail :)

vvagaytsev
vvagaytsev previously approved these changes Aug 21, 2023
Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

Amazing work! Thank you! I've left a few non-blocking comments, LGTM! 💯

Copy link
Collaborator

@vvagaytsev vvagaytsev left a comment

Choose a reason for hiding this comment

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

💯 🚀

@vvagaytsev vvagaytsev merged commit a2c5f6e into main Aug 22, 2023
2 checks passed
@vvagaytsev vvagaytsev deleted the perf/random-improvements branch August 22, 2023 14:03
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.

2 participants