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.

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

Inline trivial Entry wrapper methods and impl functions #633

Merged
merged 1 commit into from
Jun 5, 2022

Conversation

MarijnS95
Copy link
Collaborator

Looks like these went missing from #606.

@Ralith where do you generally put the cutoff point? That is, this file has some Display/Error/From impls which could get the same treatment? How about load_checked()/linked()/load()? Anything else in utils.rs that we want to cover?

@MarijnS95 MarijnS95 requested a review from Ralith June 5, 2022 11:03
@Ralith
Copy link
Collaborator

Ralith commented Jun 5, 2022

My rule of thumb is to #[inline] stuff for which the benefit is glaringly obvious (that is to say, for which the body should compile down to at most a handful of trivial instructions). For example, any case of an accessor that directly returns a (reference to a) field satisfies this.

load_checked

Generic functions are de-facto inline due to monomorphization, so I wouldn't bother.

linked/load

On the surface, these could be a good candidate, since they are trivial. However, they expand to calls to other functions, which are not all obviously trivial, so there may not be significant savings. The low likelihood of them being performance relevant acts as a tiebreaker, so I wouldn't bother.

@MarijnS95
Copy link
Collaborator Author

Sounds good.

However, they expand to calls to other functions, which are not all obviously trivial, so there may not be significant savings.

I remember reading that for those functions to be included in cross-crate optimizations, they themselves have to be marked #[inline] as well - so this would only save some trivial extra method calls. But, since they are not as trivial as the others on the surface nor are part of any critical path, let's not bother for now.

@MarijnS95 MarijnS95 merged commit 965df80 into master Jun 5, 2022
@MarijnS95 MarijnS95 deleted the entry-inline branch June 5, 2022 18:58
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