-
Notifications
You must be signed in to change notification settings - Fork 760
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
Arc-wrap PubGrubPackage
for cheap cloning in pubgrub
#3688
Conversation
f1e8fa2
to
a478060
Compare
Pubgrub stores incompatibilities as (package name, version range) tuples, meaning it needs to clone the package name for each incompatibility, and each non-borrowed operation on incompatibilities. #3673 made me realize that `PubGrubPackage` has gotten large (expensive to copy), so like `Version` and other structs, i've added an `Arc` wrapper around it. It's a pity clippy forbids `.deref()`, it's less opaque than `&**` and has IDE support (clicking on `.deref()` jumps to the right impl). ## Benchmarks It looks like this matters most for complex resolutions which, i assume because they carry larger `PubGrubPackageInner::Package` and `PubGrubPackageInner::Extra` types. ```bash hyperfine --warmup 5 "./uv-main pip compile -q ./scripts/requirements/jupyter.in" "./uv-branch pip compile -q ./scripts/requirements/jupyter.in" hyperfine --warmup 5 "./uv-main pip compile -q ./scripts/requirements/airflow.in" "./uv-branch pip compile -q ./scripts/requirements/airflow.in" hyperfine --warmup 5 "./uv-main pip compile -q ./scripts/requirements/boto3.in" "./uv-branch pip compile -q ./scripts/requirements/boto3.in" ``` ``` Benchmark 1: ./uv-main pip compile -q ./scripts/requirements/jupyter.in Time (mean ± σ): 18.2 ms ± 1.6 ms [User: 14.4 ms, System: 26.0 ms] Range (min … max): 15.8 ms … 22.5 ms 181 runs Benchmark 2: ./uv-branch pip compile -q ./scripts/requirements/jupyter.in Time (mean ± σ): 17.8 ms ± 1.4 ms [User: 14.4 ms, System: 25.3 ms] Range (min … max): 15.4 ms … 23.1 ms 159 runs Summary ./uv-branch pip compile -q ./scripts/requirements/jupyter.in ran 1.02 ± 0.12 times faster than ./uv-main pip compile -q ./scripts/requirements/jupyter.in ``` ``` Benchmark 1: ./uv-main pip compile -q ./scripts/requirements/airflow.in Time (mean ± σ): 153.7 ms ± 3.5 ms [User: 165.2 ms, System: 157.6 ms] Range (min … max): 150.4 ms … 163.0 ms 19 runs Benchmark 2: ./uv-branch pip compile -q ./scripts/requirements/airflow.in Time (mean ± σ): 123.9 ms ± 4.6 ms [User: 152.4 ms, System: 133.8 ms] Range (min … max): 118.4 ms … 138.1 ms 24 runs Summary ./uv-branch pip compile -q ./scripts/requirements/airflow.in ran 1.24 ± 0.05 times faster than ./uv-main pip compile -q ./scripts/requirements/airflow.in ``` ``` Benchmark 1: ./uv-main pip compile -q ./scripts/requirements/boto3.in Time (mean ± σ): 327.0 ms ± 3.8 ms [User: 344.5 ms, System: 71.6 ms] Range (min … max): 322.7 ms … 334.6 ms 10 runs Benchmark 2: ./uv-branch pip compile -q ./scripts/requirements/boto3.in Time (mean ± σ): 311.2 ms ± 3.1 ms [User: 339.3 ms, System: 63.1 ms] Range (min … max): 307.8 ms … 317.0 ms 10 runs Summary ./uv-branch pip compile -q ./scripts/requirements/boto3.in ran 1.05 ± 0.02 times faster than ./uv-main pip compile -q ./scripts/requirements/boto3.in ```
a478060
to
b4e9a12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you read my mind. I was thinking about doing exactly this too. I don't think I realized how much we were cloning PubGrubPackage
though. Nice find. This will probably help even more once marker
is a non-None
value. (Right now, it's always None
, so cloning will still be cheap.)
While I don't mind the &*foo
and &**foo
myself, if you wanted to go the foo.deref()
route, I'd recommend just defining an additional concrete method, e.g., foo.as_inner()
or something.
Thanks! |
Pubgrub stores incompatibilities as (package name, version range) tuples, meaning it needs to clone the package name for each incompatibility, and each non-borrowed operation on incompatibilities. #3673 made me realize that
PubGrubPackage
has gotten large (expensive to copy), so likeVersion
and other structs, i've added anArc
wrapper around it.It's a pity clippy forbids
.deref()
, it's less opaque than&**
and has IDE support (clicking on.deref()
jumps to the right impl).Benchmarks
It looks like this matters most for complex resolutions which, i assume because they carry larger
PubGrubPackageInner::Package
andPubGrubPackageInner::Extra
types.