Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@jamesqo
Copy link
Contributor

@jamesqo jamesqo commented Sep 27, 2016

Same as #12062, but for Stack. I had to add AggressiveInlining to get it to work, since it seems like the 1 extra byte from the dec in _size - 1 was making it ineligible for inlining.

Test code, before/after asm is available at the link. You can see inlining Stack.Peek increases the code size of Main by 31 bytes, while inlining Queue.Peek (asm is in the previous PR) increases it by 30.

cc @stephentoub, @ianhays

@stephentoub
Copy link
Member

Separating out the throw into its own method is fine, keeping it the same as queue. But we shouldn't add AggressiveInlining unless there's a strong performance-motivated scenario for doing so. Getting something to inline isn't a goal in and of itself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Any reason for the naming inconsistency between ThrowForEmptyQueue in #12062 and ThrowInvalidOperationEmptyStack here? It'd be nice if both methods were named consistently. I like the naming you have chosen here, so ThrowInvalidOperationEmptyQueue and ThrowInvalidOperationEmptyStack.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm partial to the one I had in the other PR. Although it's less descriptive, it's more terse. I'll remove the inconsistency if this will be merged.

@jamesqo
Copy link
Contributor Author

jamesqo commented Oct 2, 2016

@stephentoub OK, sounds reasonable. I'll remove the AggressiveInlining part of this PR tmrw and only include the part separating the throw logic into a new method. If I come up with a 'strong performance-motivated scenario' another day I'll resubmit the rest of the changes.

@jamesqo jamesqo changed the title Make Stack.Peek inline Reduce code size of Stack.Peek Oct 2, 2016
@stephentoub stephentoub merged commit a945cdd into dotnet:master Oct 3, 2016
@jamesqo jamesqo deleted the qs-inl branch October 3, 2016 01:47
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants