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

Check whether vector value needs to be dropped in Vec#truncate #57949

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@xfix
Copy link
Contributor

xfix commented Jan 28, 2019

Fixes #17633.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 28, 2019

r? @dtolnay

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

@dtolnay
Copy link
Member

dtolnay left a comment

LGTM and I think the severe O0 performance hit is worth fixing. Let me run it by one other person since this involves unsafe code:

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Jan 28, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 28, 2019

I'm personally wary and would prefer to not land changes like this in libstd at this time. I understand the tradeoff and how this performs much better at O0, but there are a number of aspects which give me pause:

  • Primarily, this is nowhere near the only part of the standard library that could benefit from treatment like this. Practically the entire standard library is built on "unnecessary" abstractions that compile away and could be hand-coded much more efficiently at O0. I'm not really sure why we'd single out just one method to get fixed.

  • Secondarily, this actually makes code at O0 somewhat worse in terms of codegen time. LLVM now has to emit this much more code (new branches) for all calls to this functions, and then LLVM has to emit code for all that.

Our somewhat ad-hoc-ly developed policy (AFAIK) is that we don't accept O0-only improvements into libstd, and I'd prefer to stick to that unless we can develop a policy to encompass more than just a handful of functions in libstd.

@xfix

This comment has been minimized.

Copy link
Contributor Author

xfix commented Jan 29, 2019

Primarily, this is nowhere near the only part of the standard library that could benefit from treatment like this.

Agreed. Even then, I don't think it's a bad idea to do that in places that can benefit from it, even if there is much more to do.

Practically the entire standard library is built on "unnecessary" abstractions that compile away and could be hand-coded much more efficiently at O0. I'm not really sure why we'd single out just one method to get fixed.

Mostly because users actually workaround around that one. See kmeisthax/rapidtar@92f4af2

Secondarily, this actually makes code at O0 somewhat worse in terms of codegen time. LLVM now has to emit this much more code (new branches) for all calls to this functions, and then LLVM has to emit code for all that.

I.e. having more tokens in a source code slows down compilation. This is technically true, although mostly meaningless.

@xfix xfix force-pushed the xfix:check-whether-vector-value-needs-to-be-dropped-in-truncate branch from 0813128 to 9f0ca59 Jan 29, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 29, 2019

I undertand how this would make linked code like above no longer necessary, but I continue to feel that this is not something we should do in libstd. Despite this being localized, this can easily snowball and has questionable value in returns. We've basically never tried to develop idioms for Rust where O0 runtime is fast, O0 is basically optimized for compile time speed and that's it.

Given basically any slow part of libstd that gets optimized away we can generate a program that "too slow" at O0, so I'm not sure we can place too much weight on any one example.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Jan 31, 2019

@xfix Is it possible your most recent force-push had the wrong content? This looks like a different change.

@xfix xfix force-pushed the xfix:check-whether-vector-value-needs-to-be-dropped-in-truncate branch from 9f0ca59 to 0813128 Jan 31, 2019

@xfix

This comment has been minimized.

Copy link
Contributor Author

xfix commented Jan 31, 2019

Yes, that's what happened.

@xfix xfix force-pushed the xfix:check-whether-vector-value-needs-to-be-dropped-in-truncate branch from 0813128 to 39ee117 Jan 31, 2019

@Dylan-DPC

This comment has been minimized.

Copy link
Member

Dylan-DPC commented Feb 11, 2019

ping from triage @alexcrichton awaiting your review on this

@dtolnay

This comment has been minimized.

Copy link
Member

dtolnay commented Feb 11, 2019

I don't foresee Alex's perspective changing. #57949 (comment) are strong arguments.

@dtolnay dtolnay closed this Feb 11, 2019

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