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

Cache absolute_path to decrease allocations #346

Merged
merged 10 commits into from Oct 4, 2021
Merged

Cache absolute_path to decrease allocations #346

merged 10 commits into from Oct 4, 2021

Conversation

mre
Copy link
Member

@mre mre commented Sep 26, 2021

While profiling local file handling, I noticed that resolving paths was taking a
significant amount of time. It also caused quite a few allocations.
By caching the path and using a constant value for the current
directory, we can reduce the number of allocs by quite a lot (5%).

This is my second attempt at reducing allocs. The first one is here #345.

While profiling local file handling, I noticed that resolving paths was taking a
significant amount of time. It also caused quite a few allocations.
By caching the path and using a constant value for the current
directory, we can reduce the number of allocs by quite a lot.
For example, when testing on the sentry documentation, we do 50,4%
less allocations in total now. That's just a single test-case of course,
but it's probably also helping in many other cases as well.
@mre
Copy link
Member Author

mre commented Sep 27, 2021

I'm running into the same issues as in idanarye/rust-typed-builder#57 again.

unresolved import `proc_macro`
Error:  --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/cached_proc_macro-0.6.1/src/lib.rs:2:5

My suspicion is that this is actually an issue in cargo-publish-all.

When comparing the compile commands between cargo publish and cargo-publish-all, I see some differences:

cargo publish --manifest-path lychee-lib/Cargo.toml --dry-run:

Running `rustc --edition=2018 --crate-name cached_proc_macro /Users/mendler/.cargo/registry/src/github.com-1ecc6299db9ec823/cached_proc_macro-0.6.1/src/lib.rs --color always --crate-type proc-macro --emit=dep-info,link -C prefer-dynamic -C debuginfo=2 -C metadata=6e1a7fdec346a337 -C extra-filename=-6e1a7fdec346a337 --out-dir /Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps -L dependency=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps --extern async_mutex=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libasync_mutex-2ee8355213f6df5e.rlib --extern cached_proc_macro_types=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libcached_proc_macro_types-a45930cd4b059174.rlib --extern darling=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libdarling-50f925d1b6481f97.rlib --extern quote=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libquote-a7e4134125a1867a.rlib --extern syn=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libsyn-d51afdcd4396af17.rlib --cap-lints allow -C link-arg=-fuse-ld=/usr/local/bin/zld`

cargo-publish-all --dry-run --verbose:

Running `rustc --crate-name cached_proc_macro --edition=2018 /Users/mendler/.cargo/registry/src/github.com-1ecc6299db9ec823/cached_proc_macro-0.6.1/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi --crate-type proc-macro --emit=dep-info,link -C prefer-dynamic -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 -C metadata=38551d9cf0baf197 -C extra-filename=-38551d9cf0baf197 --out-dir /Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps -L dependency=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps --extern async_mutex=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libasync_mutex-508e33d55550a4a0.rlib --extern cached_proc_macro_types=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libcached_proc_macro_types-5cc5ed053f71330c.rlib --extern darling=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libdarling-17ce3cd6437792f3.rlib --extern quote=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libquote-28f6537793fd29d9.rlib --extern syn=/Users/mendler/Code/private/lychee/lychee/target/package/lychee-lib-0.7.2/target/debug/deps/libsyn-33604140a69bb4fb.rlib --extern proc_macro --cap-lints allow -C link-arg=-fuse-ld=/usr/local/bin/zld`

Namely, cargo-publish-all uses --extern proc_macro.
Strangely enough, when I run the rustc command used by cargo-publish-all, it works but if I use the one from cargo publish (without --extern proc_macro) it doesn't. I was expecting it to be the other way around.

@dblock fyi

@dblock
Copy link
Contributor

dblock commented Sep 29, 2021

I'm running into the same issues as in idanarye/rust-typed-builder#57 again.

Did that just reappear and is it reproducible on main?

@mre
Copy link
Member Author

mre commented Sep 30, 2021

Oh I might not have expressed myself very well. Just to be clear, the old bug with rust-typed-builder is fixed and that fix still works. But after adding the cached crate which also uses macros, I get another compile error about proc_macro missing. It's not an issue with the rust-typed-builder or cached crates but with cargo-publish-all.

@dblock
Copy link
Contributor

dblock commented Sep 30, 2021

Oh I might not have expressed myself very well. Just to be clear, the old bug with rust-typed-builder is fixed and that fix still works. But after adding the cached crate which also uses macros, I get another compile error about proc_macro missing. It's not an issue with the rust-typed-builder or cached crates but with cargo-publish-all.

I see. You should (re)open an issue in cargo-publish-all, cc: @idanarye

@mre
Copy link
Member Author

mre commented Sep 30, 2021

Good point. I've added a comment to https://gitlab.com/torkleyy/cargo-publish-all/-/issues/3.

@mre
Copy link
Member Author

mre commented Oct 4, 2021

There was no update on the issue yet. I've decided to disable the publish --dry-run for now as it is blocking the merge and this PR reduces the amount of allocs by 20% from my measurements with cargo instruments. The runtime also improved thanks to this, but there is still a lot of low-hanging fruit around glob handling and link extraction, which we can tackle in a separate PR.

image

image

@mre mre merged commit 251332e into master Oct 4, 2021
@mre mre deleted the cache-file-resolve branch October 4, 2021 23:37
jorgelbg pushed a commit to jorgelbg/lychee that referenced this pull request Oct 6, 2021
* Cache `absolute_path` to decrease allocations

While profiling local file handling, I noticed that resolving paths was taking a
significant amount of time. It also caused quite a few allocations.
By caching the path and using a constant value for the current
directory, we can reduce the number of allocs by quite a lot.
For example, when testing on the sentry documentation, we do 50,4%
less allocations in total now. That's just a single test-case of course,
but it's probably also helping in many other cases as well.

* Defer to_string for attr.value to reduce allocs
* Use Tendrils instead of Strings for parsing (another ~1.5% less allocs)
* Move option parsing code into separate module
* Handle base dir more correctly
* Temporarily disable dry run
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.

None yet

2 participants