feat: introduce internal IdRef type #435
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are).
jalil-salame
force-pushed
the
use-idref
branch
from
2e4fd5c
to
f22f099
Compare
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Predrag Gruevski <2348618+obi1kenobi@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Jalil David Salamé Messina <60845989+jalil-salame@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Jalil David Salamé Messina <60845989+jalil-salame@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Jalil David Salamé Messina <60845989+jalil-salame@users.noreply.github.com>
obi1kenobi
added a commit
that referenced
this pull request
Sep 1, 2024
We use `&Id` in many places. As `Id` is defined as `struct Id(pub String)` this is a ptr-to-ptr situation. By using `&IdRef(str)` we remove that indirection, this increases memory usage as `&IdRef` is now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it: ```console (32418dd)❯ cargo criterion --bench indexed_crate time: [1.3590 s 1.3599 s 1.3609 s] (changes)❯ cargo criterion --bench indexed_crate IndexedCrate/new(aws-sdk-ec2) time: [1.2456 s 1.2466 s 1.2478 s] change: [-8.4413% -8.3275% -8.2222%] (p = 0.00 < 0.05) Performance has improved. (32418dd)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [571.63 ms 572.93 ms 574.23 ms] (changes)❯ cargo criterion --bench indexed_crate --features rayon IndexedCrate/new(aws-sdk-ec2) time: [501.28 ms 502.32 ms 503.34 ms] change: [-12.596% -12.324% -12.053%] (p = 0.00 < 0.05) Performance has improved. ``` The discussions on Zulip propose changing the `Id` definition in `rustdoc-types` to `u64` or even `u32`, but while we wait for this change to land (and for older `rustdoc` versions), this should be a nice perf improvement. The improvements are bigger when using `rayon`, because we spend more time on `visibility_tracker` stuff (which is where the biggest improvements are). Co-authored-by: Jalil David Salamé Messina <60845989+jalil-salame@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use
&Idin many places. AsIdis defined asstruct Id(pub String)this is a ptr-to-ptr situation. By using&IdRef(str)we remove that indirection, this increases memory usage as&IdRefis now a "wide" ptr (ptr and length) instead of a "thin" ptr, but the perf win is worth it:The discussions on Zulip propose changing the
Iddefinition inrustdoc-typestou64or evenu32, but while we wait for this change to land (and for olderrustdocversions), this should be a nice perf improvement.The improvements are bigger when using
rayon, because we spend more time onvisibility_trackerstuff (which is where the biggest improvements are).