-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Lazy load ISourceLinkService to reduce DLL loads #58108
Lazy load ISourceLinkService to reduce DLL loads #58108
Conversation
private readonly ISourceLinkService? _sourceLinkService; | ||
|
||
/// <summary> | ||
/// Lazy import ISourceLinkService because it can cause debugger |
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.
I think you forgot to
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.
it's like we're here to start eachothers...
return null; | ||
|
||
// This should ideally be the repo-relative path to the file, and come from SourceLink: https://github.com/dotnet/sourcelink/pull/699 | ||
var relativePath = Path.GetFileName(sourceDocument.FilePath); | ||
|
||
var delay = Task.Delay(SourceLinkTimeout, cancellationToken); | ||
var sourceFileTask = _sourceLinkService.GetSourceFilePathAsync(sourceDocument.SourceLinkUrl, relativePath, logger, cancellationToken); | ||
var sourceFileTask = _sourceLinkService.Value.GetSourceFilePathAsync(sourceDocument.SourceLinkUrl, relativePath, logger, cancellationToken); |
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.
Can/should this do a switch to the background thread to ensure the load happens there?
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.
(although I guess any thread switch should probably be in the implementation since this code doesn't know anything about threads like that...)
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.
Maybe. I'll leave that for @davidwengier . This is just me doing my best to play whack-a-mole on RPS regressions because of a new DLL load. Thread wouldn't reduce that problem :/
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 is just me doing my best to play whack-a-mole on RPS regressions
And I'm very grateful because I have no idea why the first commit wouldn't have solved the problem ¯\_(ツ)_/¯
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.
, and my question about moving to background thread can be treated as a separate idea.
…rovements * upstream/main: (68 commits) Lazy load ISourceLinkService to reduce DLL loads (dotnet#58108) [main] Update dependencies from dotnet/source-build (dotnet#57707) [main] Update dependencies from dotnet/arcade (dotnet#57968) Factor nullability logic for placeholders (dotnet#58036) Standardize list pattern lowering on `Index` constructor. (dotnet#58055) Add scripts to verify if a branch is ready to review Merge pull request dotnet#58100 from dotnet/dev/jorobich/skip-test Fix some places we weren't correctly disposing of VisualStudioAnalyzers Fix analyzer references being removed and added in one batch Fix indenting Ensure we don't silently capture any exceptions Don't MEF import the implementation directly if the public type will do Change comment Add comment Use actual jump tables Remove unused function Revert Simplify code Compute kind on demand Reorder ...
Validation build https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/367995