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

potential performance regression on windows #229

Closed
AxelNennker opened this issue Jul 31, 2019 · 10 comments
Closed

potential performance regression on windows #229

AxelNennker opened this issue Jul 31, 2019 · 10 comments

Comments

@AxelNennker
Copy link

Hyperledger Indy moved back from backtrace 0.3.33 to 0.3.11 because it seems the Windows performance worsened. Please see
hyperledger-archives/indy-sdk#1763

Would be nice if you could look into this. Thanks
@alexcrichton
Copy link
Member

Thanks for the report! Is there any more information to go along with this? This is unfortunately not really actionable just knowing "windows might be slow"

@AxelNennker
Copy link
Author

I don't have a Windows system to test this.
Would the benches here reveal such a regression?

Currently Indy is before a release so the core team is really busy.
I'll write a PR to libindy that uses the 0.3.33 version and see how the windows CI reacts.
Although the Jenkins jobs are currently often terminated because of the pre-release load.

Sorry, I have not more data now. // CC @Artemkaaas @KitHat

alexcrichton added a commit that referenced this issue Jul 31, 2019
@alexcrichton
Ignore return of SymInitializeW on Windows
e6ac926 
This commit updates the behavior of backtraces on Windows to execute
`SymInitializeW` globally once-per-process and ignore the return value
as to whether it succeeded or not. This undoes previous work in this
crate to specifically check the return value, and this is a behavior
update for the standard library when this goes into the standard
library.

The `SymInitializeW` function is required to be called on MSVC before
any backtraces can be captured or before any addresses can be
symbolized. This function is quite slow. It can only be called
once-per-process and there's a corresponding `SymCleanup` to undo the
work of `SymInitializeW`.

The behavior of what to do with `SymInitializeW` has changed a lot over
time in this crate. The very first implementation for Windows paired
with `SymCleanup`. Then reports of slowness at rust-lang/rustup#591
led to ac8c6d2 where `SymCleanup` was removed. This led to #165 where
because the standard library still checked `SymInitializeW`'s return
value and called `SymCleanup` generating a backtrace with this crate
would break `RUST_BACKTRACE=1`. Finally (and expectedly) the performance
report has come back as #229 for this crate.

I'm proposing that the final nail is put in this coffin at this point
where this crate will ignore the return value of `SymInitializeW`,
initializing symbols once per process globally. This update will go into
the standard library and get updated on the stable channel eventually,
meaning both libstd and this crate will be able to work with one another
and only initialize the process's symbols once globally. This does mean
that there will be a period of "breakage" where we will continue to make
`RUST_BACKTRACE=1` not useful if this crate is used on MSVC, but that's
sort of the extension of the status quo as of a month or so ago. This
will eventually be fixed once the stable channel of Rust is updated.
@alexcrichton alexcrichton mentioned this issue Jul 31, 2019
Merged
@alexcrichton
Copy link
Member

Ok, some small performance testing reveals two culprits for the change in performance, although there could be more:

  • First is Ignore return of SymInitializeW on Windows #231, which will get fixed if that's merged. The problem is that SymInitializeW, an expensive operation, is called on each call to Backtrace::new.
  • Second is that support for generating a backtrace inlined function frames on MSVC has been added. This manifests in the usage of StackWalkEx instead of StackWalk64 where supported, and this also changes the symbolication routines.

The former is relatively easily fixed, the second isn't really fixable AFAIK. A configuration option could be added to the crate for whether you want accurate or fast backtraces, but that's about the only solution there.

@AxelNennker
Copy link
Author

The Indy project is building a C-callable library libindy from Rust source code. So, I guess, we don't care about inlined function frames too much. Thanks for looking into this and publishing 0.3.34 so quickly. We'll give that a try soon.

@alexcrichton
Copy link
Member

Ok thanks for testing it out, I think if it's an acceptable level of performance we can probably leave it as-is, otherwise I can look into providing a build-time option to forcing usage of older functions on Windows.

@Artemkaaas
Copy link

I did testing of backtrace=0.3.34 on our CI (windows machine).
The usual time of running tests (with usage backtrace=0.3.11) is about ~ 23-28 minutes.
With backtrace=0.3.34 it took about 50 minutes. I tried twice.
Looks like the problem still presents for windows.

@alexcrichton
Copy link
Member

Oh wow that's quite the regression! If that jumps so much it seems like the majority of your CI time even before may have been capturing backtraces. Mentioned above I think the StackWalk64 strategy is just fundamentally faster than StackWalkEx since it parses less information. It sounds like in y'all's use case though backtraces are being generated a bit too much in the test suite?

@AxelNennker AxelNennker mentioned this issue Aug 23, 2019
Merged
@alexcrichton
Copy link
Member

I'm going to close this since it's been quite some time and I don't think this is going to fundamentally change. If there's something we can do to still get an accurate backtrace but amortize the cost though please let me know!

@caldempsey
Copy link

caldempsey commented Jun 24, 2020 •
edited
Loading

@alexcrichton not sure if this helps but I benched creating 10 million UUIDs and writing them to a file in Rust using https://github.com/mmacheerpuppy/rust-10m-uuid, the performance is worse on Windows by a magnitude of about 30, may have something to do with the filesystem support! Totally not sure if related but wanted to bring it to your attention in case this gives you a lead

@alexcrichton
Copy link
Member

@mmacheerpuppy hm that seems unrelated to this crate? Are backtraces generated there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alexcrichton @AxelNennker @caldempsey @Artemkaaas