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

Overhaul `FileSearch` and `SearchPaths` #56090

Merged
merged 6 commits into from Dec 13, 2018

Conversation

Projects
None yet
8 participants
@nnethercote
Contributor

nnethercote commented Nov 20, 2018

FileSearch::search() traverses one or more directories. For each
directory it generates a Vec<PathBuf> containing one element per file
in that directory.

In some benchmarks this occurs enough that the allocations done for the
PathBufs are significant, and in practice a small number of
directories are being traversed over and over again. For example, when
compiling the tokio-webpush-simple benchmark, two directories are
traversed 58 times each. Each of these directories have more than 100
files.

We can do all the necessary traversals up front, when Session is created,
and get the Vec<PathBuf>s then.

This reduces instruction counts on several benchmarks by 1--5%.

r? @alexcrichton

CC @eddyb, @michaelwoerister, @nikomatsakis

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 20, 2018

Here are some instruction count results.

tokio-webpush-simple-check
        avg: -2.9%      min: -4.8%      max: -1.9%
helloworld-check
        avg: -2.1%      min: -2.3%      max: -2.0%
unify-linearly-check
        avg: -1.5%      min: -2.0%      max: -1.1%
deeply-nested-check
        avg: -1.2%      min: -1.9%      max: -1.0%
sentry-cli-check
        avg: -1.0%      min: -1.8%      max: -0.5%
ctfe-stress-check
        avg: -0.1%?     min: -1.5%?     max: 1.3%?
issue-46449-check
        avg: -1.2%      min: -1.5%      max: -1.1%
regression-31157-check
        avg: -0.8%      min: -1.2%      max: -0.5%
ripgrep-check
        avg: -0.6%      min: -1.0%      max: -0.2%
coercions-check
        avg: -0.2%?     min: -0.9%?     max: 0.4%?
syn-check
        avg: -0.5%      min: -0.6%      max: -0.4%
crates.io-check
        avg: -0.4%      min: -0.6%      max: -0.1%

The wall-time numbers are roughly similar (as far as I can tell given how noisy they are). This suggests that the reading of the directory metadata isn't expensive compared to the allocations.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 20, 2018

Nice find! This code in particular is sort of insanely old, not really having changed much since 2012 basically. I think @eddyb is right in that we could probably fix this in a more first-class fashion perhaps?

I'm thinking something like:

  • Whenever a search path is pushed into a Session (or wherever it's pushed) we immediately fs::read_dir and cache the result
  • The return value of this function is impl Iterator<Item =&Path>
  • Filtering can be moved out elsewhere if need be

That way in theory we just have a borrowed iterator of paths floating around which should allocate far less?

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

I initially tried to put the cache in the Session. But the cache needs to be mutable and many immutable references are made to Session, so that quickly became untenable.

So then I tried CrateLoader, which is not as long-lived as Session, nor as widely used, and that worked well.

for_each_lib_search_path is called from several places where the CachedFilePaths aren't really necessary, such as here:

fn archive_search_paths(sess: &Session) -> Vec<PathBuf> {
let mut search = Vec::new();
sess.target_filesearch(PathKind::Native).for_each_lib_search_path(|path, _| {
search.push(path.to_path_buf());
});
search
}

In that case there is no CachedFilePaths because there is no CrateLoader -- just a Session, which is problematic, as I mentioned above.

So I'm sympathetic to wanting this to be cleaner, but I don't see how to do it. The key question is: where should the cache be stored? I can't see a better place than CrateLoader. Suggestions welcome.

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 21, 2018

@alexcrichton Pretty sure search paths are created at session creation time from the command-line arguments and never modified afterwards.

So we should be able to run all of this on all search paths at the same time.

I was thinking we'd have an "init cache at the first use" policy, but if doing it eagerly, always, seems reasonable (which I think it is, because you always end up needing them), I'm definitely for it.

CrateLoader is created only once AFAICT, maybe generate the full list in CrateLoader::new?

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

Ok, I can generate the cache when Session is created and store it within the Session. That makes things nicer. Thank you for the suggestions. I'll put up new code tomorrow.

@nnethercote nnethercote force-pushed the nnethercote:filesearch branch from 6a04166 to a8ca7f8 Nov 21, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

I have updated the code to put the cache (well, it's more of a map now) on the Session.

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 21, 2018

⌛️ Trying commit a8ca7f8 with merge e9c0d99...

bors added a commit that referenced this pull request Nov 21, 2018

Auto merge of #56090 - nnethercote:filesearch, r=<try>
Avoid regenerating the `Vec<PathBuf>` in `FileSearch::search()`.

`FileSearch::search()` traverses one or more directories. For each
directory it generates a `Vec<PathBuf>` containing one element per file
in that directory.

In some benchmarks this occurs enough that the allocations done for the
`PathBuf`s are significant, and in practice a small number of
directories are being traversed over and over again. For example, when
compiling the `tokio-webpush-simple` benchmark, two directories are
traversed 58 times each. Each of these directories have more than 100
files.

This commit adds a cache for the `Vec<PathBuf>`s to `CrateLoader`. A
reference to this cache is then passed through
`Context::maybe_load_library_crate()` and
`Context::find_library_crate()` to `FileSearch::search()`, which checks
it first before doing the traversal.

This is ugly but effective; it reduces instruction counts on several
benchmarks by 1--5%.

r? @alexcrichton

CC @eddyb, @michaelwoerister, @nikomatsakis
Show resolved Hide resolved src/librustc/session/mod.rs Outdated
@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

This is an aside about performance, for @rust-lang/wg-compiler-performance.

I re-measured performance locally on the rustc-perf benchmarks and the results were pretty good, similar to the results I showed near the top of this PR. But this time I measured both "Check" and "Debug" builds, and there were plenty of improvements for Debug builds, in many cases better than those for Check builds. Which is surprising, because this is purely a front-end change, so I would expect the Debug improvements to be uniformly smaller than the Check improvements.

So I looked in detail at tokio-webpush-simple. Here are the results for a "clean" run:

clean-check    1,700,477,965.00    1,658,074,334.00   -2.5%
clean-debug   18,780,164,569.00   18,436,798,429.00   -1.8%

Things to note:

  • The Debug build takes more than 10x longer than the Check build.
  • The Check build improved by 2.5%, which is 42 million instructions.
  • The Debug build improved by 1.8%, which is 344 million instructions.

How can that happen? Let's do a diff with Cachegrind. Here is the top part of the diff for a Check build:

-12,578,748  /build/glibc-OTsEL5/glibc-2.27/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_avx_unaligned_erms
 -4,661,049  /build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c:_int_malloc
 -2,501,425  /build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c:free
 -2,410,284  /build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c:realloc
 -2,155,253  /build/glibc-OTsEL5/glibc-2.27/elf/dl-lookup.c:do_lookup_x
 -2,101,636  /build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c:_int_realloc
 -1,684,185  /build/glibc-OTsEL5/glibc-2.27/dirent/../sysdeps/posix/readdir_r.c:readdir_r
 -1,632,722  /build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c:_int_free
 -1,490,883  /usr/include/c++/7/bitset:SetImpliedBits(llvm::FeatureBitset&, llvm::SubtargetFeatureKV const&, llvm::ArrayRef<llvm::SubtargetFeatureKV>)
 -1,413,572  /build/glibc-OTsEL5/glibc-2.27/string/../sysdeps/x86_64/multiarch/strlen-avx2.S:__strlen_avx2
  1,408,423  ???:llvm::cl::generic_parser_base::findOption(llvm::StringRef)
 -1,334,976  /home/njn/moz/rustN/src/llvm/include/llvm/Support/CommandLine.h:llvm::cl::parser<llvm::PassInfo const*>::getOption(unsigned int) const
  1,292,910  ???:SetImpliedBits(llvm::FeatureBitset&, llvm::SubtargetFeatureKV const&, llvm::ArrayRef<llvm::SubtargetFeatureKV>)
 -1,186,401  /home/njn/moz/rustN/src/llvm/lib/Support/CommandLine.cpp:llvm::cl::generic_parser_base::findOption(llvm::StringRef)
  1,182,720  ???:llvm::cl::parser<llvm::PassInfo const*>::getOption(unsigned int) const
   -976,636  /build/glibc-OTsEL5/glibc-2.27/elf/dl-lookup.c:_dl_lookup_symbol_x
   -947,594  /home/njn/moz/rustN/src/libstd/sys/unix/fs.rs:<std::sys::unix::fs::ReadDir as core::iter::iterator::Iterator>::next
   -931,509  /build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c:malloc
   -877,628  /home/njn/moz/rustN/src/libcore/iter/mod.rs:<core::iter::FilterMap<I, F> as core::iter::iterator::Iterator>::next
   -767,235  /build/glibc-OTsEL5/glibc-2.27/string/../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:__memset_avx2_unaligned_erms
   -743,648  /home/njn/moz/rustN/src/libstd/path.rs:std::path::Path::_join

It's mostly changes you'd expect from this PR: malloc/free, readdir, iteration. There's a bit of LLVM stuff which is odd, but the llvm changes are roughly equal positive and negative, so can be ignored with a bit of hand-waving.

Now, here's the top part of the diff for a Debug build:

  286,164,867  ???:llvm::calculateDbgEntityHistory(llvm::MachineFunction const*, llvm::TargetRegisterInfo const*, llvm::DbgValueHistoryMap&, llvm::DbgLabelInstrMap&)
  212,258,496  ???:llvm::FoldingSetNodeID::AddInteger(unsigned int)
  176,942,522  ???:llvm::StringMapImpl::LookupBucketFor(llvm::StringRef)
 -149,666,057  /home/njn/moz/rustN/src/llvm/include/llvm/Support/DJB.h:llvm::StringMapImpl::LookupBucketFor(llvm::StringRef)
  135,118,317  ???:llvm::MachineInstr::addOperand(llvm::MachineFunction&, llvm::MachineOperand const&)
  132,950,895  ???:llvm::SelectionDAGISel::SelectCodeCommon(llvm::SDNode*, unsigned char const*, unsigned int)
 -127,439,964  /home/njn/moz/rustN/src/llvm/lib/Support/FoldingSet.cpp:llvm::FoldingSetNodeID::AddInteger(unsigned int)
 -121,164,995  /home/njn/moz/rustN/src/llvm/lib/CodeGen/MachineInstr.cpp:llvm::MachineInstr::addOperand(llvm::MachineFunction&, llvm::MachineOperand const&)
 -114,197,798  /home/njn/moz/rustN/src/llvm/include/llvm/ADT/SmallPtrSet.h:llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AAResults*, llvm::CodeGenOpt::Level)
  112,120,861  ???:llvm::MCExpr::evaluateAsRelocatableImpl(llvm::MCValue&, llvm::MCAssembler const*, llvm::MCAsmLayout const*, llvm::MCFixup const*, llvm::DenseMap<llvm::MCSection cons
t*, unsigned long, llvm::DenseMapInfo<llvm::MCSection const*>, llvm::detail::DenseMapPair<llvm::MCSection const*, unsigned long> > const*, bool) const
 -107,678,177  /home/njn/moz/rustN/src/llvm/lib/MC/MCExpr.cpp:llvm::MCExpr::evaluateAsRelocatableImpl(llvm::MCValue&, llvm::MCAssembler const*, llvm::MCAsmLayout const*, llvm::MCFixup
const*, llvm::DenseMap<llvm::MCSection const*, unsigned long, llvm::DenseMapInfo<llvm::MCSection const*>, llvm::detail::DenseMapPair<llvm::MCSection const*, unsigned long> > const*, bo
ol) const
 -104,916,811  /home/njn/moz/rustN/src/llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:llvm::calculateDbgEntityHistory(llvm::MachineFunction const*, llvm::TargetRegisterInfo
 const*, llvm::DbgValueHistoryMap&, llvm::DbgLabelInstrMap&)
  104,708,489  ???:llvm::FoldingSetNodeIDRef::ComputeHash() const
 -104,211,697  /home/njn/moz/rustN/src/llvm/include/llvm/ADT/Hashing.h:std::enable_if<llvm::hashing::detail::is_hashable_data<unsigned int const>::value, llvm::hash_code>::type llvm::h
ashing::detail::hash_combine_range_impl<unsigned int const>(unsigned int const*, unsigned int const*)
 -104,066,881  /home/njn/moz/rustN/src/llvm/include/llvm/ADT/DenseMap.h:llvm::SelectionDAG::Combine(llvm::CombineLevel, llvm::AAResults*, llvm::CodeGenOpt::Level)
  103,304,706  ???:llvm::SmallPtrSetImplBase::FindBucketFor(void const*) const
   93,639,002  ???:(anonymous namespace)::Verifier::visitInstruction(llvm::Instruction&)
   93,540,485  ???:llvm::FoldingSetBase::FindNodeOrInsertPos(llvm::FoldingSetNodeID const&, void*&)
   93,154,223  ???:(anonymous namespace)::RegAllocFast::allocateBasicBlock(llvm::MachineBasicBlock&)
   92,373,726  ???:(anonymous namespace)::LiveDebugValues::transferRegisterDef(llvm::MachineInstr&, (anonymous namespace)::LiveDebugValues::OpenRangesSet&, llvm::UniqueVector<(anonymou
s namespace)::LiveDebugValues::VarLoc> const&) [clone .isra.277]
  -90,815,761  /home/njn/moz/rustN/src/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:llvm::SelectionDAGISel::SelectCodeCommon(llvm::SDNode*, unsigned char const*, unsigned int)
  -86,226,661  /home/njn/moz/rustN/src/llvm/lib/Support/SmallPtrSet.cpp:llvm::SmallPtrSetImplBase::FindBucketFor(void const*) const
  -84,838,899  /home/njn/moz/rustN/src/llvm/include/llvm/ADT/BitVector.h:llvm::calculateDbgEntityHistory(llvm::MachineFunction const*, llvm::TargetRegisterInfo const*, llvm::DbgValueHi
storyMap&, llvm::DbgLabelInstrMap&)
  -81,949,764  /home/njn/moz/rustN/src/llvm/lib/IR/Attributes.cpp:llvm::Attribute::hasAttribute(llvm::StringRef) const
   81,949,764  ???:llvm::Attribute::hasAttribute(llvm::StringRef) const
  -80,775,116  /home/njn/moz/rustN/src/llvm/lib/Support/FoldingSet.cpp:llvm::FoldingSetBase::FindNodeOrInsertPos(llvm::FoldingSetNodeID const&, void*&)
   80,208,052  ???:(anonymous namespace)::X86MCCodeEmitter::encodeInstruction(llvm::MCInst const&, llvm::raw_ostream&, llvm::SmallVectorImpl<llvm::MCFixup>&, llvm::MCSubtargetInfo cons
t&) const

What a mess. And it's all LLVM stuff, swamping the malloc/free/readdir changes from the front-end. If the positives and negatives balanced out, I'd probably just shrug my shoulders. But somehow it ends up benefiting us by 300 million instructions, and I don't know why. Any thoughts?

@bors

This comment has been minimized.

Contributor

bors commented Nov 22, 2018

☀️ Test successful - status-travis
State: approved= try=True

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 22, 2018

@rust-timer

This comment has been minimized.

rust-timer commented Nov 22, 2018

Success: Queued e9c0d99 with parent 910ec6d, comparison URL.

@rust-timer

This comment has been minimized.

rust-timer commented Nov 22, 2018

Finished benchmarking try commit e9c0d99

@nnethercote nnethercote force-pushed the nnethercote:filesearch branch from a8ca7f8 to af1aaf5 Nov 22, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 22, 2018

@eddyb: A new, map-free version is up.

@nnethercote nnethercote force-pushed the nnethercote:filesearch branch from af1aaf5 to 6ad4b3c Nov 22, 2018

Show resolved Hide resolved src/librustc/session/mod.rs Outdated
@bors

This comment has been minimized.

Contributor

bors commented Dec 11, 2018

⌛️ Testing commit 4b70b01 with merge a1746bd...

bors added a commit that referenced this pull request Dec 11, 2018

Auto merge of #56090 - nnethercote:filesearch, r=eddyb
Overhaul `FileSearch` and `SearchPaths`

`FileSearch::search()` traverses one or more directories. For each
directory it generates a `Vec<PathBuf>` containing one element per file
in that directory.

In some benchmarks this occurs enough that the allocations done for the
`PathBuf`s are significant, and in practice a small number of
directories are being traversed over and over again. For example, when
compiling the `tokio-webpush-simple` benchmark, two directories are
traversed 58 times each. Each of these directories have more than 100
files.

We can do all the necessary traversals up front, when `Session` is created,
and get the `Vec<PathBuf>`s then.

This reduces instruction counts on several benchmarks by 1--5%.

r? @alexcrichton

CC @eddyb, @michaelwoerister, @nikomatsakis
@eddyb

This comment has been minimized.

Member

eddyb commented Dec 11, 2018

@nnethercote I don't think try runs any tests, we should probably rename it.

@bors

This comment has been minimized.

Contributor

bors commented Dec 11, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Dec 11, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:32:30] doc tests for: /checkout/src/doc/rustdoc/src/unstable-features.md
[01:32:30] 
[01:32:30] running 4 tests
[01:32:30] test /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Extensions_to_the_::Add_aliases_for_an_item_in_documentation_search (line 205) ... ok
[01:32:30] thread '<unnamed>' panicked at 'failed to acquire jobserver token: Bad file descriptor (os error 9)', src/librustc_codegen_ssa/back/write.rs:1358:29
[01:32:30] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:32:30] thread '<unnamed>' panicked at 'failed to acquire jobserver token: Bad file descriptor (os error 9)', src/librustc_codegen_ssa/back/write.rs:1358:29
[01:32:30] thread '<unnamed>' panicked at 'failed to acquire jobserver token: Bad file descriptor (os error 9)', src/librustc_codegen_ssa/back/write.rs:1358:29
[01:32:30] test /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Nightly_gated_functionality::Linking_to_items_by_type (line 68) ... FAILED
[01:32:30] test /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Extensions_to_the_::Documenting_platform__feature_specific_information (line 118) ... FAILED
[01:32:30] 
[01:32:30] failures:
[01:32:30] failures:
[01:32:30] 
[01:32:30] ---- /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Nightly_gated_functionality::Linking_to_items_by_type (line 54) stdout ----
[01:32:30] error: failed to acquire jobserver token: Bad file descriptor (os error 9)
[01:32:30] thread '/checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Nightly_gated_functionality::Linking_to_items_by_type (line 54)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:326:13
[01:32:30] 
[01:32:30] ---- /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Nightly_gated_functionality::Linking_to_items_by_type (line 68) stdout ----
[01:32:30] ---- /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Nightly_gated_functionality::Linking_to_items_by_type (line 68) stdout ----
[01:32:30] error: failed to acquire jobserver token: Bad file descriptor (os error 9)
[01:32:30] thread '/checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Nightly_gated_functionality::Linking_to_items_by_type (line 68)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:326:13
[01:32:30] 
[01:32:30] ---- /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Extensions_to_the_::Documenting_platform__feature_specific_information (line 118) stdout ----
[01:32:30] ---- /checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Extensions_to_the_::Documenting_platform__feature_specific_information (line 118) stdout ----
[01:32:30] error: failed to acquire jobserver token: Bad file descriptor (os error 9)
[01:32:30] thread '/checkout/src/doc/rustdoc/src/unstable-features.md - Unstable_features::Extensions_to_the_::Documenting_platform__feature_specific_information (line 118)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:326:13
[01:32:30] 
[01:32:30] 
[01:32:30] failures:
---
[01:32:30] 
[01:32:30] 
[01:32:30] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:32:30] Build completed unsuccessfully in 0:43:53
[01:32:30] Makefile:58: recipe for target 'check' failed
[01:32:30] make: *** [check] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:13aad6fc
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Dec 11 12:08:30 UTC 2018
---
travis_time:end:30f553d6:start=1544530112413004922,finish=1544530112422321554,duration=9316632
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:064f2b7c
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0419e3cd
travis_time:start:0419e3cd
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:103dc082
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nnethercote added some commits Nov 20, 2018

Avoid regenerating the `Vec<PathBuf>` in `FileSearch::search()`.
`FileSearch::search()` traverses one or more directories. For each
directory it generates a `Vec<PathBuf>` containing one element per file
in that directory.

In some benchmarks this occurs enough that the allocations done for the
`PathBuf`s are significant, and in practice a small number of
directories are being traversed over and over again. For example, when
compiling the `tokio-webpush-simple` benchmark, two directories are
traversed 58 times each. Each of these directories have more than 100
files.

This commit changes things so that all the `Vec<PathBuf>`s that will be
needed by a `Session` are precomputed when that `Session` is created;
they are stored in `SearchPath`. `FileSearch` gets a reference to the
necessary `SearchPath`s. This reduces instruction counts on several
benchmarks by 1--5%.

The commit also removes the barely-used `visited_dirs` hash in
`for_each_lib_searchPath`. It only detects if `tlib_path` is the same as
one of the previously seen paths, which is unlikely.
Remove `Session::sysroot()`.
Instead of maybe storing its own sysroot and maybe deferring to the one
in `Session::opts`, just clone the latter when necessary so one is
always directly available. This removes the need for the getter.
Introduce `SearchPath` and replace `SearchPaths` with `Vec<SearchPath>`.
It's more idiomatic, makes the code shorter, and will help with the next
commit.
Replace `FileSearch::for_each_lib_search_path` with `search_paths`.
Returning an iterator leads to nicer code all around.
@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Dec 12, 2018

This is the error:

[01:32:30] thread '<unnamed>' panicked at 'failed to acquire jobserver token: Bad file descriptor (os error 9)', src/librustc_codegen_ssa/back/write.rs:1358:29

Which is somehow hit during rustdoc processing. That error message has been seen multiple times in the past, e.g. #42867, https://bugzilla.mozilla.org/show_bug.cgi?id=1457583. Updating rustc or sccache or something else has at times made it go away again.

This PR in theory does not change functional behaviour, other than it causes directories to be traversed once instead of multiple times.

@alexcrichton, @glandium: any ideas? If not, my plan is to do a clumsy bisection by landing the commits one at a time in separate PRs, so I can at least narrow the problem down to a single commit.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

Er sorry, I meant to help out taking a look into this earlier!

That error message typically means that there's a preexisting bug somewhere else that ended up being discovered here. Knowing about that error message and looking at this patch, my guess is that this bug only surfaces during doctests where we're running multiple instances of rustc in parallel in the same process (multiple Session instances). This means that Session is being created concurrently with another test probably getting codegen'd, and the shift in where file descriptors are analyzed here is probably affecting that (something about file descriptors being opened earlier on or something like that).

Now all that being said if that panic is hit then it's a bug in the configuration of the build system. Something above rustc is telling it "hey look at these file descriptors for synchronization" but those file descriptors are actually invalid. Typically rustbuild, when building the compiler, removes relevant env vars to ensure Cargo has control over everything (and we rarely inherit proper configuration in the build system.

I think the bug here though is that this is happening during doctests that aren't spawned by Cargo, but rather rustdoc is spawned manually. If you add similar env_remove calls when we call rustdoc I think it may fix this?

Remove some env vars for rustdoc invocations.
In an attempt to avoid "thread '<unnamed>' panicked at 'failed to
acquire jobserver token: Bad file descriptor" errors.

@nnethercote nnethercote force-pushed the nnethercote:filesearch branch from 4b70b01 to 209240d Dec 12, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Dec 12, 2018

@alexcrichton: Goodness! I will take your word for it.

I've added a new commit, does that match your expectations?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 12, 2018

I swear I have reasons for my unreasonable demands!

@bors r=eddyb

@bors

This comment has been minimized.

Contributor

bors commented Dec 12, 2018

📌 Commit 209240d has been approved by eddyb

@bors

This comment has been minimized.

Contributor

bors commented Dec 13, 2018

⌛️ Testing commit 209240d with merge ced7cc5...

bors added a commit that referenced this pull request Dec 13, 2018

Auto merge of #56090 - nnethercote:filesearch, r=eddyb
Overhaul `FileSearch` and `SearchPaths`

`FileSearch::search()` traverses one or more directories. For each
directory it generates a `Vec<PathBuf>` containing one element per file
in that directory.

In some benchmarks this occurs enough that the allocations done for the
`PathBuf`s are significant, and in practice a small number of
directories are being traversed over and over again. For example, when
compiling the `tokio-webpush-simple` benchmark, two directories are
traversed 58 times each. Each of these directories have more than 100
files.

We can do all the necessary traversals up front, when `Session` is created,
and get the `Vec<PathBuf>`s then.

This reduces instruction counts on several benchmarks by 1--5%.

r? @alexcrichton

CC @eddyb, @michaelwoerister, @nikomatsakis
@bors

This comment has been minimized.

Contributor

bors commented Dec 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing ced7cc5 to master...

@bors bors merged commit 209240d into rust-lang:master Dec 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Dec 13, 2018

Hurray it worked!

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Dec 13, 2018

Holy smokes, it did!

@nnethercote nnethercote deleted the nnethercote:filesearch branch Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment