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

Eliminate boxing allocation #8888

Merged
merged 1 commit into from Feb 28, 2021
Merged

Conversation

drewnoakes
Copy link
Member

Proposed changes

This change removes 537,404 bytes of boxed enum allocations while loading GE with the gitextensions repo.

Test methodology

  • Manual testing

✒️ I contribute this code under The Developer Certificate of Origin.

This change removes 537,404 bytes of boxed enum allocations while loading GE with the gitextensions repo.
@ghost ghost assigned drewnoakes Feb 25, 2021
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #8888 (75eae19) into master (b6905f1) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8888      +/-   ##
==========================================
- Coverage   56.03%   56.03%   -0.01%     
==========================================
  Files         922      922              
  Lines       65944    65944              
  Branches    12070    12070              
==========================================
- Hits        36951    36950       -1     
- Misses      25985    25986       +1     
  Partials     3008     3008              
Flag Coverage Δ
production 43.25% <100.00%> (-0.01%) ⬇️
tests 94.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Comment on lines -889 to +891
if (!AppSettings.ShowGitNotes && refFilterOptions.HasFlag(RefFilterOptions.All | RefFilterOptions.Boundary))
if (!AppSettings.ShowGitNotes && (refFilterOptions & gitNotesOptions) == gitNotesOptions)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC .NET Core has resolved this allocations, hasn't it? #8522 makes this change obsolete.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's not needed under .NET Core. What is the timeline for porting?

Until such a time, I suggest merging this. I ran a trace over GE startup and this was a quick win from the top 30 types allocated on the heap.

Copy link
Member

@gerhardol gerhardol Feb 25, 2021

Choose a reason for hiding this comment

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

then this should go into 3.5 too
.NET5 is likely in next version

Copy link
Member Author

Choose a reason for hiding this comment

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

Sibling PR targeting 3.5: #8889

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 25, 2021
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Feb 25, 2021
@RussKie

This comment has been minimized.

@gerhardol

This comment has been minimized.

@RussKie
Copy link
Member

RussKie commented Feb 25, 2021 via email

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

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

+1

@RussKie RussKie merged commit 17c23c3 into gitextensions:master Feb 28, 2021
@ghost ghost added this to the 3.6 milestone Feb 28, 2021
@RussKie RussKie modified the milestones: 3.6, 3.5 Feb 28, 2021
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.

None yet

3 participants