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
Revision reader tweak #11170
Revision reader tweak #11170
Conversation
Avoid replace \v in email etc Make revision reading slightly easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
public (string? subject, string? body, bool hasMultiLineMessage) PeekSubjectBody(bool skipBody) | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you decide this attribute is necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This routine can be executed 100K every time the revisions are reloaded, so speed is preferred over code size
I considered removing the internal class StringLineReader to give the compiler some more hints.
I doubt this is measurable though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Proposed changes
Avoid replace \v in email etc (not needed, performance).
Make revision reading slightly easier to read.
IDE0008 in RevisionReader.cs
This originated as a review to see if it was possible to improve the performance, primarily to see if allocations could be eliminated, but it does not seem there is big difference.
Still, this is critical code when opening repos.
Test methodology
Manual
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.