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

Jump to bottom

On disk cache serialization #6094

Merged
merged 8 commits into from Feb 25, 2021
Merged

On disk cache serialization #6094

merged 8 commits into from Feb 25, 2021

Conversation

rokonec
Copy link
Contributor

@rokonec rokonec commented Jan 27, 2021 •
edited

Context

BinaryFormater serialization of on-disk RAR cache is slow and unsecure.
Related issue: #6057

Changes Made

Serialization changed to use custom binary format by using existing ITranslatable

Testing

Unit testing
Manual testing

  • Roslyn repo rebuild + incremental build

Notes

@Forgind
Copy link
Member

Forgind commented Jan 28, 2021

Looks roughly good to me, and as I said over Teams—don't worry about the mono failures.

@Forgind Forgind mentioned this pull request Feb 2, 2021
Merged
@rokonec rokonec self-assigned this Feb 2, 2021
@rokonec rokonec marked this pull request as ready for review February 2, 2021 22:47
@rokonec rokonec requested a review from Forgind February 2, 2021 22:48
Forgind
Forgind approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple questions and some nits.

src/Build.UnitTests/BackEnd/BinaryTranslator_Tests.cs Show resolved Hide resolved
src/Build.UnitTests/BackEnd/BinaryTranslator_Tests.cs Show resolved Hide resolved
src/Build.UnitTests/BackEnd/BinaryTranslator_Tests.cs Outdated Show resolved Hide resolved
src/Build.UnitTests/BackEnd/BinaryTranslator_Tests.cs Outdated Show resolved Hide resolved
src/Shared/TranslatorHelpers.cs Outdated Show resolved Hide resolved
src/Tasks/SystemState.cs Outdated Show resolved Hide resolved
src/Tasks/SystemState.cs Outdated Show resolved Hide resolved
src/Tasks/SystemState.cs Outdated Show resolved Hide resolved
src/Tasks/SystemState.cs Outdated Show resolved Hide resolved
src/Tasks/TaskTranslatorHelpers.cs Show resolved Hide resolved
@cdmihai
Copy link
Contributor

cdmihai commented Feb 4, 2021

Very curious to see what the time differences are between this and master. Please post numbers once you have them :)

@cdmihai

This comment has been minimized.

Sign in to view
cdmihai
cdmihai approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@cdmihai cdmihai left a comment

Choose a reason for hiding this comment

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

General approach looks good. Curios to see perf data comparisons.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 6, 2021
@rokonec
Copy link
Contributor Author

rokonec commented Feb 8, 2021 •
edited

@cdmihai

General approach looks good. Curios to see perf data comparisons.

Perf data:

  • On rotational DISK incremental build of Roslyn is about 8% shorter at RAR wall clock time.
  • On SSD incremental build of OrchardCore is about 2% shorter at RAR wall clock time.
  • RAR cache on-disk size is about 5x smaller (20% of original size).
  • Memory allocated by RAR in incremental Roslyn build is about 15% smaller.

@ladipro ladipro merged commit 6819f7a into dotnet:master Feb 25, 2021
@marcpopMSFT marcpopMSFT mentioned this pull request Mar 2, 2021
Closed
@rokonec rokonec deleted the rokonec/6057-on-disk-cache-serialization branch April 1, 2021 13:37
@Forgind Forgind mentioned this pull request Apr 15, 2021
Merged
@Forgind Forgind mentioned this pull request Apr 29, 2021
Closed
@ladipro ladipro mentioned this pull request Feb 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdmihai cdmihai cdmihai approved these changes

@Forgind Forgind Forgind approved these changes

Assignees

@rokonec rokonec

Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rokonec @Forgind @cdmihai @ladipro