-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,11 +20,16 @@ namespace Microsoft.CodeAnalysis.PdbSourceDocument | |
internal sealed class PdbSourceDocumentLoaderService : IPdbSourceDocumentLoaderService | ||
{ | ||
private const int SourceLinkTimeout = 1000; | ||
private readonly ISourceLinkService? _sourceLinkService; | ||
|
||
/// <summary> | ||
/// Lazy import ISourceLinkService because it can cause debugger | ||
/// binaries to be eagerly loaded even if they are never used. | ||
/// </summary> | ||
private readonly Lazy<ISourceLinkService?> _sourceLinkService; | ||
|
||
[ImportingConstructor] | ||
[SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code")] | ||
public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkService? sourceLinkService) | ||
public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] Lazy<ISourceLinkService?> sourceLinkService) | ||
{ | ||
_sourceLinkService = sourceLinkService; | ||
} | ||
|
@@ -105,14 +110,14 @@ public PdbSourceDocumentLoaderService([Import(AllowDefault = true)] ISourceLinkS | |
|
||
private async Task<SourceFileInfo?> TryGetSourceLinkFileAsync(SourceDocument sourceDocument, Encoding encoding, IPdbSourceDocumentLogger? logger, CancellationToken cancellationToken) | ||
{ | ||
if (_sourceLinkService is null || sourceDocument.SourceLinkUrl is null) | ||
if (sourceDocument.SourceLinkUrl is null || _sourceLinkService.Value is null) | ||
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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
And I'm very grateful because I have no idea why the first commit wouldn't have solved the problem ¯\_(ツ)_/¯ |
||
|
||
var winner = await Task.WhenAny(sourceFileTask, delay).ConfigureAwait(false); | ||
|
||
|
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...