Speed up the fast path for assert_eq! and assert_ne! by dotdash · Pull Request #57815 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation28 Commits1 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

dotdash

Currently, the panic!() calls directly borrow the value bindings. This
causes those bindings to always be initialized, i.e. they're initialized
even before the values are even compared. This causes noticeable
overhead in what should be a really cheap operation.

By performing a reborrow of the value in the call to panic!(), we allow
LLVM to optimize that code, so that the extra borrow only happens in the
error case.

We could achieve the same result by dereferencing the values passed to
panic!(), as the format machinery borrows them anyway, but this causes
assertions to fail to compile if one of the values is unsized, i.e. it
would be a breaking change.

@dotdash

Currently, the panic!() calls directly borrow the value bindings. This causes those bindings to always be initialized, i.e. they're initialized even before the values are even compared. This causes noticeable overhead in what should be a really cheap operation.

By performing a reborrow of the value in the call to panic!(), we allow LLVM to optimize that code, so that the extra borrow only happens in the error case.

We could achieve the same result by dereferencing the values passed to panic!(), as the format machinery borrows them anyway, but this causes assertions to fail to compile if one of the values is unsized, i.e. it would be a breaking change.

@rust-highfive

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@dotdash

@bors try

Local numbers seem promising, but I'd like to have "official" numbers from prlo as well

screen shot 2019-01-21 at 19 44 26-fullpage

@bors

bors added a commit that referenced this pull request

Jan 21, 2019

@bors

Speed up the fast path for assert_eq! and assert_ne!

Currently, the panic!() calls directly borrow the value bindings. This causes those bindings to always be initialized, i.e. they're initialized even before the values are even compared. This causes noticeable overhead in what should be a really cheap operation.

By performing a reborrow of the value in the call to panic!(), we allow LLVM to optimize that code, so that the extra borrow only happens in the error case.

We could achieve the same result by dereferencing the values passed to panic!(), as the format machinery borrows them anyway, but this causes assertions to fail to compile if one of the values is unsized, i.e. it would be a breaking change.

@dotdash

FWIW, the above numbers are on top of a build with my pending PRs already applied.

@sfackler

Would it be worth adding a codegen test to make sure these continue to behave nicely?

@dotdash

Would it be worth adding a codegen test to make sure these continue to behave nicely?

Good idea. I'll try to add one tomorrow. I will have to check how to do so, it's been a while and I'm out of time for today.

@dotdash

Oh, just in case somebody's wondering about it, a good deal of the perf win is probably due to the fact that functions using those macros are now smaller, which allows for them to be inlined, pontentially avoiding even more copies. This was the case for the to_bits function, which I had been investigating, and which wasn't inlined even though it had the inline attribute which even lowers the bar for inlining. With those changes, the function is properly inlined.

@bors

@hellow554

Maybe my issue will be solved then? #55914

@dotdash

For most practical purposes it should be fixed. There will still be a slight difference because of the stack space needed for the formatting of the panic message, but that's just the adjustment of the stack pointer, which will probably happen anyway in many cases because of the stack needed by function using the assertion and is the price you pay for getting a more detailed error message. Am Di., 22. Jan. 2019, 09:50 hat Marcel Hellwig notifications@github.com geschrieben:

…

@dotdash

Actually, I just checked again, and there's still some extra work happening that's not strictly required.

Assembly looks like this:

assert_eq!

pushq	%rax
.cfi_def_cfa_offset 16
cmpl	%esi, %edi
jne	.LBB10_1
movb	$1, %al
popq	%rcx
.cfi_def_cfa_offset 8
retq

assert_eq! (new)

subq	$104, %rsp
.cfi_def_cfa_offset 112
movl	%edi, (%rsp)
movl	%esi, 4(%rsp)
cmpl	%esi, %edi
jne	.LBB11_1
movb	$1, %al
addq	$104, %rsp
.cfi_def_cfa_offset 8
retq

😢

@RalfJung

@rust-timer

@rust-timer

Finished benchmarking try commit 261660d

@dotdash

Hm, that's interesting, the number's aren't even close to what I got here. I'll check later whether there is some interaction between this and my other PRs that were included on both sides here.

@dotdash

Seems that my baseline compiler was somehow slower than it should be, after cleaning out the build directory of that one and doing a fresh build (x.py build on its own didn't do anything), I get the same numbers as prlo. Sorry for providing wrong nunmbers initially.

I'm experimenting with an improved version of this to get (closer to) a result that is more or less equal to the code that assert(a == b), possibly outlining the panic call as well, but there are currently some roadblocks to that, so it might take a while (or even fail).

Do we want to merge this anyway (possibly with an adapted commit message) or shall I close it for now?

@hellow554

I did some benchmarks with outlining the panic. It will give me this:

example::unlkly: pushq %rax cmpl %esi, %edi jne .LBB2_1 popq %rcx retq .LBB2_1: callq example::moep ud2

IMHO, this should be even more simplified to

example::unlkly: cmpl %esi, %edi jne .LBB2_1 retq .LBB2_1: # [doing stuff] callq example::moep ud2

Either way, I don't understand why rust/LLVM does pushq %rax and popq %rcx. Is it a bug?

@dotdash

Either way, I don't understand why rust/LLVM does pushq %rax and popq %rcx. Is it a bug?

No, these instructions adjust %rsp to get the correct stack alignment. Using those push/pops leads to shorter instructions than doing a sub on %rsp itself.

@sfackler

So it sounds like there's still a performance improvement from this change, it's just smaller than originally thought? SGTM if that's the case.

@hellow554

Either way, I don't understand why rust/LLVM does pushq %rax and popq %rcx. Is it a bug?

No, these instructions adjust %rsp to get the correct stack alignment. Using those push/pops leads to shorter instructions than doing a sub on %rsp itself.

I don't think that this applies here, because you don't need to align the stack at all... Just do a compare, branch (if needed) or return.

@dotdash

Either way, I don't understand why rust/LLVM does pushq %rax and popq %rcx. Is it a bug?

No, these instructions adjust %rsp to get the correct stack alignment. Using those push/pops leads to shorter instructions than doing a sub on %rsp itself.

I don't think that this applies here, because you don't need to align the stack at all... Just do a compare, branch (if needed) or return.

You can skip the alignment step in leaf functions, but this is not a leaf function, there's a call to example::moep. And IIRC (just vaguely though) the stack pointer adjustment has to happen in the function prologue, so you can't move it past the branch.

@Dylan-DPC-zz

ping from triage @sfackler awaiting your review on this

@sfackler

@bors

📌 Commit 5a7cd84 has been approved by sfackler

@bors bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 12, 2019

Centril added a commit to Centril/rust that referenced this pull request

Feb 13, 2019

@Centril

Speed up the fast path for assert_eq! and assert_ne!

Currently, the panic!() calls directly borrow the value bindings. This causes those bindings to always be initialized, i.e. they're initialized even before the values are even compared. This causes noticeable overhead in what should be a really cheap operation.

By performing a reborrow of the value in the call to panic!(), we allow LLVM to optimize that code, so that the extra borrow only happens in the error case.

We could achieve the same result by dereferencing the values passed to panic!(), as the format machinery borrows them anyway, but this causes assertions to fail to compile if one of the values is unsized, i.e. it would be a breaking change.

Centril added a commit to Centril/rust that referenced this pull request

Feb 13, 2019

@Centril

Speed up the fast path for assert_eq! and assert_ne!

Currently, the panic!() calls directly borrow the value bindings. This causes those bindings to always be initialized, i.e. they're initialized even before the values are even compared. This causes noticeable overhead in what should be a really cheap operation.

By performing a reborrow of the value in the call to panic!(), we allow LLVM to optimize that code, so that the extra borrow only happens in the error case.

We could achieve the same result by dereferencing the values passed to panic!(), as the format machinery borrows them anyway, but this causes assertions to fail to compile if one of the values is unsized, i.e. it would be a breaking change.

bors added a commit that referenced this pull request

Feb 13, 2019

@bors

Rollup of 13 pull requests

Successful merges:

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request

Feb 13, 2019

@Centril

Speed up the fast path for assert_eq! and assert_ne!

Currently, the panic!() calls directly borrow the value bindings. This causes those bindings to always be initialized, i.e. they're initialized even before the values are even compared. This causes noticeable overhead in what should be a really cheap operation.

By performing a reborrow of the value in the call to panic!(), we allow LLVM to optimize that code, so that the extra borrow only happens in the error case.

We could achieve the same result by dereferencing the values passed to panic!(), as the format machinery borrows them anyway, but this causes assertions to fail to compile if one of the values is unsized, i.e. it would be a breaking change.

bors added a commit that referenced this pull request

Feb 13, 2019

@bors

Rollup of 12 pull requests

Successful merges:

Failed merges:

r? @ghost

@rocallahan

And IIRC (just vaguely though) the stack pointer adjustment has to happen in the function prologue, so you can't move it past the branch.

I don't think that's true. Stack alignment only has to be correct when you call into a function. Seems like LLVM needs an optimization to defer aligning the stack if the hot code contains no function calls.

@hellow554

Stack alignment only has to be correct when you call into a function

IIRC some instructions also need an aligned stack. It's a weird scenario after all.

@rocallahan

Some SSE instructions require aligned data addresses. The compiler can easily handle this.

@hellow554

But then it would require an LLVM update, I'm afraid there's not much rust can do.

@krdln krdln mentioned this pull request

Feb 21, 2019

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.