Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync #72229

ToddGrun · 2024-02-22T02:50:53Z

This code realized an ImmutableArray before from an ArrayBuilder. This immutablearray was only used within the scope of SynchronizeAssetsAsync.

*** Prior allocations from speedometer:

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs

…llocations

sharwell · 2024-02-22T17:17:57Z

src/Workspaces/Core/Portable/Serialization/PooledList.cs

@@ -15,7 +14,7 @@ namespace Microsoft.CodeAnalysis.Serialization
    /// </summary>
    internal static class Creator
    {
-        public static PooledObject<HashSet<Checksum>> CreateChecksumSet(ImmutableArray<Checksum> checksums)
+        public static PooledObject<HashSet<Checksum>> CreateChecksumSet(SegmentedList<Checksum> checksums)


We don't want a mutable type here.

Suggested change

public static PooledObject<HashSet<Checksum>> CreateChecksumSet(SegmentedList<Checksum> checksums)

public static PooledObject<HashSet<Checksum>> CreateChecksumSet(ImmutableSegmentedList<Checksum> checksums)

I'd love for this to be an immutable type, but I don't see how to get one down to this level without allocating up in SynchronizeAssetsAsync. Maybe I'm missing a trick?

@ToddGrun what about, as an alternative, a readonly struct wrapper around SegmentedList?

I'm not clear on what that would gain. Yes, the wrapper would be immutable, but I assume it would give access to the mutable structure. I don't mind the current approach, especially since it shows promising allocation results, but am willing to consider other approaches.

a readonly struct wrapper around SegmentedList?

ImmutableSegmentedList<T> is exactly that.

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs

sharwell · 2024-02-22T17:39:29Z

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs

@@ -118,28 +122,29 @@ public async ValueTask SynchronizeProjectAssetsAsync(ProjectStateChecksums proje

        using (Logger.LogBlock(FunctionId.AssetService_SynchronizeAssetsAsync, Checksum.GetChecksumsLogInfo, checksums, cancellationToken))
        {
-            using var _1 = ArrayBuilder<Checksum>.GetInstance(checksums.Count, out var missingChecksums);
+            using var missingChecksumsObject = s_checksumListPool.GetPooledObject();


This intermediate collection is completely unnecessary. Just use the checksums argument directly:

// Remove all values from 'checksums' where the asset cache already contains a value checksums.RemoveWhere(checksum => _assetCache.TryGetAsset<object>(checksum, out _));

This can be further optimized by defining PooledDelegates.GetPooledPredicate, if necessary:

// Remove all values from 'checksums' where the asset cache already contains a value using (var _1 = PooledDelegates.GetPooledPredicate<Checksum, AssetProvider>( static (checksum, arg) => arg._assetCache.TryGetAsset<object>(checksum, out _), this, out var predicate)) { checksums.RemoveWhere(predicate); }

I don't think I'm following this suggestion.

Modifying that hashset in place requires knowledge that the caller is ok with that. It might be ok, but I'd need to verify that.

Assuming that were ok, then I'd need to pass that HashSet down and change all the calls to take in a HashSet instead of a SegmentedList, right?

Modifying that hashset in place requires knowledge that the caller is ok with that. It might be ok, but I'd need to verify that.

I did verify it, but we'd want a way to make it apparent to the callers. One option is to make the caller responsible for filtering the list, e.g. by checking the asset cache directly in the code that is adding items to the set.

Assuming that were ok, then I'd need to pass that HashSet down and change all the calls to take in a HashSet instead of a SegmentedList, right?

That's one option. We could also create an ImmutableArray from it directly by calling ImmutableArray.CreateRange<T>, without going through the builder pool.

The intent of this PR was to investigate removing the allocation of creating that ImmutableArray. If we need that data structure to be passed down as immutable, I should just close out this PR. But, from what I saw, it didn't need to be immutable, it was just a nice to have. This is an investigation into whether removing that nice to have could have a positive perf impact without too much ugliness in the code.

As for the suggestion to modify the HashSet in-place, I guess I'm still not seeing the value. Perhaps the speedometer profile with this change will show that's an issue, but I'd prefer to not make that change unless it's needed as it does change the semantics around the hashset passed into this method.

as these are all internal components, without a surface area exposed to other features, i would be fine with this not being immutable, esp if it cuts down on churn.

ToddGrun · 2024-02-23T03:55:48Z

This looks effective from the speedometer run here (CompletionTest.Completion/9990.Totals/CLR_BytesAllocates_NonDevenv)

Name                                   	Exc %	       Exc
Type Microsoft.CodeAnalysis.Checksum[] 	  0.1	 4,900,776

No HashSet<Checksum> appears in the profile.

Going to elevate out of draft status

ToddGrun · 2024-02-23T03:58:05Z

@sharwell, @CyrusNajmabadi -- I've moved this out of draft status as it shows promise, ptal

sharwell

Requesting changes pending investigation on not losing argument immutability

…List

…llocations

ToddGrun · 2024-02-27T17:13:19Z

Requesting changes pending investigation on not losing argument immutability

Would like to ping @CyrusNajmabadi and @sharwell for preferences on how to proceed. I personally don't mind sending the mutable types through the callstack as all the bits they proceed through are internal.

As I know there is concern with that approach, I investigated what would be needed to make them immutable (ImmutableSegmentedList instead of ImmutableArray). It can be done (branch here), but it's a bit more effort:

Added msgpack serialization for ImmutableSegmentedList
Added ImmutableCollectionsMarshal equivalent for ImmutableSegmentedList

I'm not in love with these changes, but I couldn't come up with an approach for sending an immutabletype through without allocations otherwise.

…nc_Allocations_ImmutableCollectionsMarshal Dev/toddgrun/synchronize assets async allocations immutable collections marshal

…llocations

ToddGrun · 2024-03-05T01:56:06Z

Requesting changes pending investigation on not losing argument immutability

@sharwell -- I'd like to keep this moving along. Do you think the ValueBuilder PR will go in sometime soon, or should I plan on making progress on this PR without that? I'm mostly interested in the ImmutableCollectionsMarshal equivalent that this PR created a rudimentary version of. (I'm also intereseted in the ImmutableSegmentedDictionary.AddRange optimization that I merged 72220 without)

ToddGrun · 2024-03-06T17:03:41Z

waiting on #72395 as it provides the ImmutableCollectionsMarshal this PR wants

sharwell · 2024-03-19T19:00:44Z

src/Dependencies/Collections/ImmutableSegmentedList`1.cs

+        public static class ImmutableCollectionsMarshal
+        {
+            public static ImmutableSegmentedList<T> ToImmutableSegmentedList(SegmentedList<T> list)
+            {
+                return new ImmutableSegmentedList<T>(list);
+            }
+        }


❗ This can be removed now

sharwell · 2024-03-19T19:03:51Z

src/Workspaces/Remote/Core/Serialization/ImmutableSegmentedListFormatter.cs

+        // TODO: Should there be a CreateBuilder that takes in an initial capacity
+        // similar to what ImmutableArray allows?


Yes, there should. In the meantime, you can use SegmentedCollectionsMarshal.AsImmutableSegmentedList if you want.

I have a branch ready to submit for this as soon as #62193 merges.

Reduce allocations under RemoveAssetProvider.SynchronizeAssetsAsync

6cf4930

CyrusNajmabadi reviewed Feb 22, 2024

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 22, 2024

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs Outdated Show resolved Hide resolved

ToddGrun added 3 commits February 22, 2024 05:21

Merge branch 'dotnet:main' into dev/toddgrun/SynchronizeAssetsAsync_A…

5cdf609

…llocations

less verbose

15594b5

Utilize the trimOnFree paramter

e108f46

sharwell changed the title ~~WIP: Reduce allocations under RemoveAssetProvider.SynchronizeAssetsAsync~~ WIP: Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync Feb 22, 2024

sharwell reviewed Feb 22, 2024

View reviewed changes

src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 22, 2024

View reviewed changes

Remove unnecessary cast

48442e7

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 23, 2024

ToddGrun marked this pull request as ready for review February 23, 2024 03:55

ToddGrun requested a review from a team as a code owner February 23, 2024 03:55

CyrusNajmabadi approved these changes Feb 23, 2024

View reviewed changes

ToddGrun changed the title ~~WIP: Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync~~ Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync Feb 23, 2024

sharwell requested changes Feb 23, 2024

View reviewed changes

ToddGrun added 3 commits February 23, 2024 15:03

WIP: use ImmutableCollectionsMarshal concept for (Immutable)Segmented…

ef3d2e0

…List

Implement messagepack formatter for ImmutableSegmentedList

5e35d64

Merge branch 'dotnet:main' into dev/toddgrun/SynchronizeAssetsAsync_A…

daa45ad

…llocations

ToddGrun added 6 commits February 27, 2024 13:29

Merge pull request #2 from ToddGrun/dev/toddgrun/SynchronizeAssetsAsy…

ef94d6f

…nc_Allocations_ImmutableCollectionsMarshal Dev/toddgrun/synchronize assets async allocations immutable collections marshal

Merge branch 'dotnet:main' into dev/toddgrun/SynchronizeAssetsAsync_A…

c3f2636

…llocations

move classes to separate files

e1a55ba

some cleanup

605097c

Add test

54d39f8

Merge branch 'main' into dev/toddgrun/SynchronizeAssetsAsync_Allocations

137822b

sharwell reviewed Mar 19, 2024

View reviewed changes

ToddGrun closed this Apr 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync #72229

Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync #72229

ToddGrun commented Feb 22, 2024

sharwell Feb 22, 2024

ToddGrun Feb 22, 2024

CyrusNajmabadi Feb 22, 2024

ToddGrun Feb 23, 2024

sharwell Feb 23, 2024

sharwell Feb 22, 2024

ToddGrun Feb 22, 2024

sharwell Feb 22, 2024 •

edited

Loading

ToddGrun Feb 22, 2024

CyrusNajmabadi Feb 22, 2024

ToddGrun commented Feb 23, 2024

ToddGrun commented Feb 23, 2024

sharwell left a comment

ToddGrun commented Feb 27, 2024

ToddGrun commented Mar 5, 2024

ToddGrun commented Mar 6, 2024

sharwell Mar 19, 2024 •

edited

Loading

sharwell Mar 19, 2024

sharwell Mar 19, 2024

	public static PooledObject<HashSet<Checksum>> CreateChecksumSet(SegmentedList<Checksum> checksums)
	public static PooledObject<HashSet<Checksum>> CreateChecksumSet(ImmutableSegmentedList<Checksum> checksums)

		// TODO: Should there be a CreateBuilder that takes in an initial capacity
		// similar to what ImmutableArray allows?

Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync #72229

Reduce allocations under RemoteAssetProvider.SynchronizeAssetsAsync #72229

Conversation

ToddGrun commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun commented Feb 23, 2024

ToddGrun commented Feb 23, 2024

sharwell left a comment

Choose a reason for hiding this comment

ToddGrun commented Feb 27, 2024

ToddGrun commented Mar 5, 2024

ToddGrun commented Mar 6, 2024

sharwell Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Feb 22, 2024 •

edited

Loading

sharwell Mar 19, 2024 •

edited

Loading