-
Notifications
You must be signed in to change notification settings - Fork 842
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
perf: Optimize read MIME operation #9132
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #9132 +/- ##
==========================================
+ Coverage 77.58% 77.59% +0.01%
==========================================
Files 591 591
Lines 24567 24578 +11
==========================================
+ Hits 19060 19071 +11
Misses 5507 5507
☔ View full report in Codecov by Sentry. |
src/Docfx.Common/YamlMime.cs
Outdated
@@ -12,10 +15,8 @@ public static class YamlMime | |||
public const string TableOfContent = YamlMimePrefix + nameof(TableOfContent); | |||
public const string XRefMap = YamlMimePrefix + nameof(XRefMap); | |||
|
|||
public static string ReadMime(TextReader reader) | |||
public static string? ReadMime([NotNull] TextReader reader) |
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.
IIUC, [NotNull]
on the parameter means that, if reader
were null, then ReadMime would throw rather than return. So that if another method calls ReadMime and passes a possibly-null value, then the compiler can assume the value is not null after ReadMime returns.
Is that the purpose for which you use [NotNull]
here?
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.
As you pointed out. [NotNull]
is not works as expected.
In Anyway it's public API. parameter's null checks are required.
So I've remove [NotNull] attribute and revert original null check codes.
e47caed
to
06e6704
Compare
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.
Thank you @filzrev !
What included in this PR
0
(Default:4096)128
(Default:1024, Min:128)FileOptions.SequentialScan
hint for Windows (It seems don't affects actual performance thought)Background
YamlMime.ReadMime
API read file's first line only.So it can reduce internal buffer allocations. and reduce Gen0 GC allocations.
Benchmark results