Remove some allocations under HttpFileSystemBasedFindPackageByIdResource.ConsumeFlatContainerIndexAsync #6265
Conversation
microsoft-github-policy-service
bot
added
the
Community
| var normalizedVersionString = Identity.Version.ToNormalizedString(); | ||
| string idInLowerCase = Id.ToLowerInvariant(); | ||
|
|
||
| var builder = StringBuilderPool.Shared.Rent(256); |
There was a problem hiding this comment.
Roughly 50% of the string builders used in this codepath end up exceeding the 256 char limit, and thus aren't released back to the pool. I didn't address it in this PR, as delay calculating this ends up getting rid of the vast majority of the work, but it might be worth considering upping that 256 char limit used in StringBuilderPool.
|
Test insertion based on commit 1 to get speedometer numbers: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/610228 |
microsoft-github-policy-service
bot
added
the
Status:No recent activity
jeffkl
removed
the
Status:No recent activity
…rce.ConsumeFlatContainerIndexAsync
jeffkl
force-pushed
the
2-11-allocations
branch
from
d6a59c6 to
cb9bd46
Compare
|
I have pushed a change on behalf of the PR author to make |
| NuGet.Common.PublicStrings | ||
| ~static NuGet.Common.PublicStrings.Culture.get -> System.Globalization.CultureInfo | ||
| ~static NuGet.Common.PublicStrings.Culture.set -> void | ||
| ~static NuGet.Common.PublicStrings.Invalid_AttributeValue.get -> string |
There was a problem hiding this comment.
this is just my personal opinion, but I don't like resource strings as public APIs. There's too much risk of runtime failures if new {x} parameters are added to the string, and callers of the API don't update their string.Format argument list. We've even had this type of failure within the repo, so it'll be much worse if anyone using the NuGet Client SDK packages in their own app use this string for some reason.
I don't think that duplicating the strings, and keeping them internal, in multiple assemblies is bad enough to prefer public APIs.
There was a problem hiding this comment.
See https://github.com/NuGet/NuGet.Client/pull/6265/files#r1970768798 for one complexity of not having these as public.
I totally understand its unusual here, but these are the only options I could come up with:
- Have a new
.resxin any assembly that needs shared code under a common namespace so that the shared code can compile against it. - Hardcode this string and not have it localized at all
- Have a set of public strings in
NuGet.Common.dllthat we are confident won't change and are pretty generic (this one for example is just telling a user that an attribute value is invalid). InternalsVisibleTo/friend assembly. I wish you could grant friend access to individual classes...- Make
Utf8JsonStreamReaderpublic so that our assemblies can share it - Refactor entire assemblies and combine them
- Other?
In this case, it seemed like the least worst option.
There was a problem hiding this comment.
I want 6, but let's wait on that until we have a major version.
I'd prefer 1
There was a problem hiding this comment.
Okay I've pushed an update to use a shared resource that gets compiled into each assembly that needs it. At least the string is only defined in one place.
Bug
Fixes: NuGet/Home#14095
Description
Reduces allocation in
HttpFileSystemBasedFindPackageByIdResource.ConsumeFlatContainerIndexAsyncUtf8JsonStreamReaderto thebuild\Sharedlocation so it can be compiled into multiple assembliesUtf8JsonStreamReader.Utf8JsonStreamReaderfor reading some JSON inHttpFileSystemBasedFindPackageByIdResource.PackageInfo.ContentUrito be calculated on a deferred basis as it's very expensive to calculate and is used quite infrequently (I saw about 0.1% ofPackageInfoobjects created actually get queried for that data)PR Checklist