Skip to content

Make time::Instant::actually_monotonic() a const fn. #65954

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

Closed
wants to merge 1 commit into from

Conversation

shamiao
Copy link
Contributor

@shamiao shamiao commented Oct 30, 2019

No description provided.

@rust-highfive
Copy link
Contributor

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2019
@kennytm
Copy link
Member

kennytm commented Oct 30, 2019

Could you provide any context why this is needed? AFAIK actually_monotonic() is only used in Instant::now() which won't be const.

@shamiao
Copy link
Contributor Author

shamiao commented Oct 30, 2019

This PR mainly prevents anyone from accidentally adding code which introduces runtime penalty to actually_monotonic().

This also removes an unnecessary call to actually_monotonic() at every time Instant::now() is called for debug builds. (EDIT: doesn't make sense. thanks @sinkuu for pointing out)

note: AFAIK monotonicity only depends on platform and build configurations and won't change at runtime.

@sinkuu
Copy link
Contributor

sinkuu commented Oct 30, 2019

This also removes an unnecessary call to actually_monotonic() at every time Instant::now() is called for debug builds.

Compile-time evaluation does not happen when const fn is called from non-const context:

const fn foo() -> bool { true }
pub fn bar() -> bool { foo() }
; playground::foo
; Function Attrs: nonlazybind uwtable
define internal zeroext i1 @_ZN10playground3foo17h3b9711775736012fE() unnamed_addr #0 !dbg !5 {
start:
  ret i1 true, !dbg !10
}

; playground::bar
; Function Attrs: nonlazybind uwtable
define zeroext i1 @_ZN10playground3bar17hfc942ebdcf035a2cE() unnamed_addr #0 !dbg !11 {
start:
; call playground::foo
  %0 = call zeroext i1 @_ZN10playground3foo17h3b9711775736012fE(), !dbg !12
  br label %bb1, !dbg !12

bb1:                                              ; preds = %start
  ret i1 %0, !dbg !13
}

EDIT: Also, std is compiled in release mode, so actually_monotonic() call should be aleady inlined.

@shamiao
Copy link
Contributor Author

shamiao commented Oct 30, 2019

@sinkuu Sorry, this is my mistake. But I wonder why the compiler doesn't evaluate it?

@sinkuu
Copy link
Contributor

sinkuu commented Oct 30, 2019

@shamiao const fn just enables functions "to be called in constants contexts". Evaluating const fn in non-const context is rather an optimization, and compiler devs seem to have the plan (#64996 (comment)).

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2019
@joelpalmer
Copy link

Ping from Triage: Any updates @shamiao

@shamiao
Copy link
Contributor Author

shamiao commented Nov 4, 2019

@joelpalmer I have no further updates. Waiting the Rust crew to decide whether this PR should be merged.

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@kennytm Can you please review/merge this PR?
CC: @shamiao

Thanks!

@kennytm
Copy link
Member

kennytm commented Nov 9, 2019

IMO the reason making this a const fn isn't compelling enough, as

  1. this function is never used in const context
  2. this function is private
  3. the optimizer will certainly evaluate and inline these trivial functions, regardless of constness

So I'd prefer to close this PR.

@shamiao
Copy link
Contributor Author

shamiao commented Nov 9, 2019

@kennytm Opinion 3 seems compelling to me. I'll close this PR.

@shamiao shamiao closed this Nov 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants