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.

Sign up for GitHub

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

Jump to bottom

Benchmark digest subsetting #14569

Closed
wants to merge 1 commit into from
Closed

Benchmark digest subsetting #14569

wants to merge 1 commit into from

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Feb 23, 2022

Probably not for merging.

[ci skip-rust]
[ci skip-build-wheels]

@benjyw benjyw marked this pull request as draft February 23, 2022 09:21
@benjyw benjyw mentioned this pull request Feb 23, 2022
Closed
benjyw
benjyw commented Feb 23, 2022
View reviewed changes
src/python/pants/backend/python/util_rules/local_dists.py
DigestSubset(
request.sources.source_files.snapshot.digest, PathGlobs(sorted(remaining_sources))
),
DigestSubset(request.sources.source_files.snapshot.digest, PathGlobs(remaining_sources)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PathGlobs ctor sorts for us, so no need to do it here ourselves

@stuhood
Copy link
Member

stuhood commented Mar 2, 2022

The results, for posterity:

  • generating the "full subset" by passing all paths in the digest in as the requested subset) takes ~1 second for 40k files and this time grows linearly with the number of files in the digest.
  • Creating a digest from file content in memory (via CreateDigest) takes ~8.3 seconds for 40k files, and this time grows roughly quadratically with the number of files in the digest.

So it is possible that full subset generation times of a few hundred ms add up across many consumers. But this benchmarking effort incidentally reveals something worse and not directly related to subsetting, i.e., superlinear behavior of CreateDigest.

https://user-images.githubusercontent.com/512764/155292114-d587c31c-af17-49e9-84a7-098c94a02e2e.png

@benjyw
Benchmark digest subsetting
38867bb 
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

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

Stale?

@benjyw
Copy link
Contributor Author

benjyw commented Aug 18, 2022

I don't know that the underlying issue has been addressed?

@stuhood
Copy link
Member

stuhood commented Aug 18, 2022

I don't know that the underlying issue has been addressed?

Opening a ticket for the CreateDigest non-linearity is probably the next step rather than leaving this open.

@stuhood stuhood mentioned this pull request Aug 18, 2022
Closed
@stuhood
Copy link
Member

stuhood commented Aug 18, 2022

Opened #16570.

@stuhood stuhood closed this Aug 18, 2022
@danxmoran danxmoran mentioned this pull request Aug 23, 2022
Merged
stuhood pushed a commit that referenced this pull request Aug 24, 2022
@danxmoran
Optimize CreateDigest implementation. (#16617)
cc3e78b 
Closes #16570

* Use a `DigestTrie` to create all snapshots at once, instead of creating them individually
* Store all in-memory file contents in a single (batched) call, instead of storing them individually

[ci skip-build-wheels]

---

I restored the benchmark script from #14569 to test this.

| size | create_digest_before | create_digest_after |
| --- | --- | --- |
| 20000 | 608 | 130 |
| 40000 | 1164 | 268 |
| 60000 | 2260 | 475 |
| 80000 | 3582 | 674 |
| 100000 | 5085 | 862 |
| 120000 | 6765 | 1057 |
| 140000 | 8818 | 1067 |
| 160000 | 10752 | 1361 |
| 180000 | 12619 | 1604 |
Eric-Arellano pushed a commit to Eric-Arellano/pants that referenced this pull request Aug 24, 2022
@danxmoran @Eric-Arellano
Optimize CreateDigest implementation. (pantsbuild#16617)
5f27ea1 
Closes pantsbuild#16570

* Use a `DigestTrie` to create all snapshots at once, instead of creating them individually
* Store all in-memory file contents in a single (batched) call, instead of storing them individually

[ci skip-build-wheels]

---

I restored the benchmark script from pantsbuild#14569 to test this.

| size | create_digest_before | create_digest_after |
| --- | --- | --- |
| 20000 | 608 | 130 |
| 40000 | 1164 | 268 |
| 60000 | 2260 | 475 |
| 80000 | 3582 | 674 |
| 100000 | 5085 | 862 |
| 120000 | 6765 | 1057 |
| 140000 | 8818 | 1067 |
| 160000 | 10752 | 1361 |
| 180000 | 12619 | 1604 |
This was referenced Aug 24, 2022
Closed
Merged
Eric-Arellano added a commit that referenced this pull request Aug 29, 2022
@Eric-Arellano
Optimize CreateDigest implementation. (Cherry-pick of #16617) (#16648)
5105dc1 
Closes #16570

* Use a `DigestTrie` to create all snapshots at once, instead of creating them individually
* Store all in-memory file contents in a single (batched) call, instead of storing them individually

[ci skip-build-wheels]

---

I restored the benchmark script from #14569 to test this.

| size | create_digest_before | create_digest_after |
| --- | --- | --- |
| 20000 | 608 | 130 |
| 40000 | 1164 | 268 |
| 60000 | 2260 | 475 |
| 80000 | 3582 | 674 |
| 100000 | 5085 | 862 |
| 120000 | 6765 | 1057 |
| 140000 | 8818 | 1067 |
| 160000 | 10752 | 1361 |
| 180000 | 12619 | 1604 |
cczona pushed a commit to cczona/pants that referenced this pull request Sep 1, 2022
@danxmoran @cczona
Optimize CreateDigest implementation. (pantsbuild#16617)
94935bd 
Closes pantsbuild#16570

* Use a `DigestTrie` to create all snapshots at once, instead of creating them individually
* Store all in-memory file contents in a single (batched) call, instead of storing them individually

[ci skip-build-wheels]

---

I restored the benchmark script from pantsbuild#14569 to test this.

| size | create_digest_before | create_digest_after |
| --- | --- | --- |
| 20000 | 608 | 130 |
| 40000 | 1164 | 268 |
| 60000 | 2260 | 475 |
| 80000 | 3582 | 674 |
| 100000 | 5085 | 862 |
| 120000 | 6765 | 1057 |
| 140000 | 8818 | 1067 |
| 160000 | 10752 | 1361 |
| 180000 | 12619 | 1604 |
@benjyw benjyw deleted the benchmark_digests branch May 24, 2023 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benjyw @stuhood @Eric-Arellano