Address PR feedback for change adding key to KeyNotFoundException#15234
Conversation
|
@Anipik Could you please write better commit description than just https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/contributing.md#commit-messages has some tips on how to write good commit messages. |
| internal static void ThrowKeyNotFoundException(object key) | ||
| { | ||
| throw new KeyNotFoundException(key.ToString()); | ||
| throw new KeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); |
There was a problem hiding this comment.
Could you please change this to:
internal static void ThrowKeyNotFoundException(object key)
{
throw GetKeyNotFoundException(object key);
}
to be consistent with the other similar methods? It will let the JIT to generate better code.
There was a problem hiding this comment.
@jkotas I understand why the ThrowXX method is valuable - the comment explains. Why is it found helpful to further break out constructing the exception?
There was a problem hiding this comment.
JIT will look into small methods to observe what they do. If the method always throws exception, the JIT will move the call to it to the "cold" section of the code and it will also know that the method will never return.
Breaking out constructing of the exception allows this optimization to kick in. The exception construction is large enough piece of code to prevents the optimization from kicking in.
Check the discussion in #7580 that introduced this optimization.
| internal static void ThrowKeyNotFoundException(object key) | ||
| { | ||
| throw new KeyNotFoundException(key.ToString()); | ||
| throw GetKeyNotFoundException(SR.Format(SR.Arg_KeyNotFoundWithKey, key.ToString())); |
There was a problem hiding this comment.
You are doing this twice now. It should be done inside the Get... method only.
danmoseley
changed the title
|
@Anipik I improved the title. |
|
@jkotas is this Pr ready to merge ? |
…tnet#15234) * Throwing KeyNotFoundException using Throw + get for JIT to generate better code for ii
3 participants
issue- dotnet/corefx#5188
corefx pr - dotnet/corefx#25472
Related Coreclr Pr - #15201