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

Fix remote cache writes memory exhaustion. (#12087) #12089

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented May 19, 2021

This builds upon #12083 which halved memory consumption when uploading
blobs to the remote cache and sets an upper bound on memory consumed for
uploads via two measures:

  • Blobs larger than a certain size are buffered via disk.
  • Uploading duplicate (large) blobs is largely avoided.

The latter is achieved with a best-effort, low cost sieve. We could do
better.

With these changes, a full fresh remote cache upload of Pants via
./pants --remote-cache-write tests :: can now complete (previously it
was OOMKilled) and does so while consuming at least 30x less memory.

(cherry picked from commit 68921c2)

[ci skip-build-wheels]

This builds upon pantsbuild#12083 which halved memory consumption when uploading
blobs to the remote cache and sets an upper bound on memory consumed for
uploads via two measures:

+ Blobs larger than a certain size are buffered via disk.
+ Uploading duplicate (large) blobs is largely avoided.

The latter is achieved with a best-effort, low cost sieve. We could do
better.

With these changes, a full fresh remote cache upload of Pants via
`./pants --remote-cache-write tests ::` can now complete (previously it
was OOMKilled) and does so while consuming at least 30x less memory.

(cherry picked from commit 68921c2)

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@jsirois
Copy link
Contributor Author

jsirois commented May 19, 2021

In case this is deemed safe enough for 2.5.1rc0, putting up the cherry-pick now.

@tdyas
Copy link
Contributor

tdyas commented May 19, 2021

In case this is deemed safe enough for 2.5.1rc0, putting up the cherry-pick now.

I assume this PR only affects remote cache writes? If so, then it has my vote for cherry-pick since having a large percentage of runs killed for OOM is pretty major bug for that feature.

@tdyas
Copy link
Contributor

tdyas commented May 19, 2021

Is this the cherry-pick PR? The PR says it it to be merged into 2.5.x but the PR title does not say "Cherry-pick of XXX".

@jsirois
Copy link
Contributor Author

jsirois commented May 19, 2021

I always cherry pick via git cherry-pick -x <sha> and I never alter the generated commit message. The two clues that leaves are the (#12087) at the end of the subject line (this PR is #12089) and the (cherry picked from commit 68921c2) In the body.

@tdyas
Copy link
Contributor

tdyas commented May 19, 2021

I believe it is more of a Pants project style question. cc @Eric-Arellano

@jsirois
Copy link
Contributor Author

jsirois commented May 19, 2021

Yeah. Eric has been accepting this style in reviews for awhile. From me at least.

@Eric-Arellano
Copy link
Contributor

Yeah, we simply update the release's changelog PR title to do the "right thing".

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thank you, John. This looks great.

Will merge given that I think you're AFK.

@Eric-Arellano Eric-Arellano merged commit c2c900f into pantsbuild:2.5.x May 24, 2021
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.

3 participants