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

perf: Change existing struct to readonly struct #9119

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

filzrev
Copy link
Contributor

@filzrev filzrev commented Aug 22, 2023

What's included in this PR

  • Change struct model to readonly struct.

This change ensure model is not modified after created.
And It can reduce object copy when passing struct as argument. and defensive copy of struct object.
(Thought It's not measurable perf improvement.)

@KalleOlaviNiemitalo
Copy link

It's anyway boxed when it is passed as IFileLinkInfo.

@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 97.05% and no project coverage change.

Comparison is base (2de8644) 77.58% compared to head (26e0969) 77.58%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9119   +/-   ##
=======================================
  Coverage   77.58%   77.58%           
=======================================
  Files         591      591           
  Lines       24567    24567           
=======================================
  Hits        19060    19060           
  Misses       5507     5507           
Files Changed Coverage Δ
src/Docfx.Common/FileAbstractLayer/PathMapping.cs 75.00% <50.00%> (-8.34%) ⬇️
...ine/TemplateProcessors/TemplateModelTransformer.cs 85.26% <100.00%> (ø)
....Build.TableOfContents/TocDocumentProcessorBase.cs 92.20% <100.00%> (ø)
src/Docfx.Common/FileLinkInfo.cs 94.87% <100.00%> (+2.56%) ⬆️
src/Docfx.Plugins/LinkSourceInfo.cs 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KalleOlaviNiemitalo
Copy link

interface IFileLinkInfo seems to be used only for ICustomHrefGenerator.GenerateHref, for which there are no built-in implementations except in a test. So the boxing can only happen when a third-party plugin is involved.

@filzrev
Copy link
Contributor Author

filzrev commented Aug 23, 2023

There is another FileInfo issue that it contains GroupInfo reference type.
So FileLinkInfo is not considered as immutable object.
And it fails to compare structure equity when GroupInfo property have value.

var result = new FileLinkInfo(fromFileInSource, fromFileInDest, href, context);
Assert.Equal(result, expected);

It might to be better to change FileLinkInfo to class. And implement IEquatable<T> for FileInfo (and GroupInfo).

@yufeih yufeih merged commit 35d4c69 into dotnet:main Aug 23, 2023
7 checks passed
@filzrev filzrev deleted the perf-change-to-readonly-struct branch August 24, 2023 02:46
p-kostov pushed a commit to ErpNetDocs/docfx that referenced this pull request Jun 28, 2024
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.

3 participants