Skip to content

Conversation

Kixiron
Copy link
Contributor

@Kixiron Kixiron commented Apr 19, 2021

  • Removed the mint() and mint_ref() methods in favor of pub(crate) new() constructors
  • Implemented Debug for ActivateCapability & Activations
  • Added .try_{delayed, downgrade} methods to allow fallibly performing delays/downgrades. Good for debugging code and for applications that can't tolerate panicking
  • Worked on the panics used within .delayed() & .downgrade() with the aim of reducing their code bloat as much as possible, thereby making the compiler eagerly inline the actual function's body without including all the panicking/formatting/generic machinery within it
  • Removed #[inline] attributes from many frequently-called functions. This aims to counteract code bloat, as the #[inline] attribute has large compile time costs (reference, rust core team member)

@frankmcsherry
Copy link
Member

I think this is mostly good, except the removal of the inline attribute. Without this attribute, compiled code may not present the Rust code that would allow for inlining, preventing the inlining short of something like LTO. In this case, #[inline(always)] is probably appropriate, as each call is just a trampoline to another call.

@Kixiron
Copy link
Contributor Author

Kixiron commented Apr 20, 2021

Generic functions are always valid for inlining regardless of attributes, but the inking attribute is highly detrimental to compile times as it makes a separate copy of each function at every invocation site. The inline attribute only affects cross crate inlining in debug mode, which is a mode I'd argue should prioritize compile times above fairly minor performance impacts. Rustc and LLVM are really smart and will inline trivial things like this, the attribute just hurts users. If debug perf is a real issue for people (which it would have been before this), they can use something like ThinLTO to get incremental LTO

@frankmcsherry
Copy link
Member

Right, but none of the PartialOrder implementations are generic. These are things like u64::less_equal which we do want to inline to be <=.

@Kixiron
Copy link
Contributor Author

Kixiron commented Apr 20, 2021

Ah, gotcha. I'll add those back to the macro, my bad

@frankmcsherry
Copy link
Member

Thanks!

@frankmcsherry frankmcsherry merged commit b619080 into TimelyDataflow:master Apr 20, 2021
@Kixiron Kixiron deleted the capabilities branch April 20, 2021 21:18
@github-actions github-actions bot mentioned this pull request Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants