Move memory-mapped-text storage system to use the new pattern introduced for memory-mapped-stream storage. by CyrusNajmabadi · Pull Request #73218 · dotnet/roslyn

CyrusNajmabadi · 2024-04-24T18:49:14Z

Updates to follow the patterns here: #73115

Followup to: #73216

CyrusNajmabadi · 2024-04-24T18:50:03Z

...ces/Core/Portable/TemporaryStorage/TemporaryStorageService.DirectMemoryAccessStreamReader.cs

+
+internal sealed partial class TemporaryStorageService
+{
+    private unsafe class DirectMemoryAccessStreamReader : TextReaderWithLength


Just a move into its own file. this didn't change at all (besides becoming nested and private).

CyrusNajmabadi · 2024-04-24T18:50:24Z

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorage.cs

-/// <summary>
-/// TemporaryStorage can be used to read and write text to a temporary storage location.
-/// </summary>
-internal interface ITemporaryTextStorageInternal


CyrusNajmabadi · 2024-04-24T18:50:38Z

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageWithName.cs

-/// <summary>
-/// TemporaryStorage can be used to read and write text to a temporary storage location.
-/// </summary>
-internal interface ITemporaryStorageWithName


CyrusNajmabadi · 2024-04-24T18:54:24Z

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

    /// The <see cref="SourceText"/> in the current process.
    /// </summary>
    /// <remarks>
-    /// <inheritdoc cref="Storage"/>


This was referencing the namespace... whoopsie :)

CyrusNajmabadi · 2024-04-24T18:55:44Z

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs

+        var identifier = new TemporaryStorageIdentifier(memoryMappedInfo.Name, memoryMappedInfo.Offset, memoryMappedInfo.Size);
+        return new(this, memoryMappedInfo.MemoryMappedFile, identifier, text.ChecksumAlgorithm, text.Encoding, text.GetContentHash());
+
+        MemoryMappedInfo WriteToMemoryMappedFile()


this code was inlined from the old TemporaryTextStorage type (which was deleted).

CyrusNajmabadi · 2024-04-24T18:56:06Z

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs

+        var identifier = new TemporaryStorageIdentifier(memoryMappedInfo.Name, memoryMappedInfo.Offset, memoryMappedInfo.Size);
+        return new(memoryMappedInfo.MemoryMappedFile, identifier);
+
+        MemoryMappedInfo WriteToMemoryMappedFile()


this code was inlined from TemporaryStreamStorage (which was itself deleted).

CyrusNajmabadi · 2024-04-24T18:56:28Z

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs

-    }
-}
-
-internal unsafe class DirectMemoryAccessStreamReader : TextReaderWithLength


moved to new file.

CyrusNajmabadi · 2024-04-24T18:56:45Z

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs

        }
    }
-
-    internal sealed class TemporaryStreamStorage


this abstraction just wasnt' necessary. i inlined it all into the one caller.

CyrusNajmabadi · 2024-04-24T20:12:44Z

...paces/Core/Portable/TemporaryStorage/TemporaryStorageService.TemporaryStorageStreamHandle.cs

+
+                var info = new MemoryMappedInfo(memoryMappedFile, Identifier.Name, Identifier.Offset, Identifier.Size);
+                return info.CreateReadableStream();
+            }


this is just the old storage.ReadStream method inlined (so we could delete TemporaryStreamStorage).

CyrusNajmabadi · 2024-04-24T20:13:56Z

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs

+        {
+            await Task.Yield().ConfigureAwait(false);
+            cancellationToken.ThrowIfCancellationRequested();
+        }


existing logic. just inlined.

ToddGrun · 2024-04-24T22:58:46Z

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorage.cs

    Task WriteStreamAsync(Stream stream, CancellationToken cancellationToken = default);
 }
-
-/// <summary>


Have the interfaces and their implementations above been obsoleted long enough for removal?

We have to make then Obsolete(error: true) first. i can do that in a followup.

opened: #73226

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageService.cs

ToddGrun · 2024-04-25T02:15:26Z

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageTextHandle.cs

+
+namespace Microsoft.CodeAnalysis.Host;
+
+internal interface ITemporaryStorageTextHandle


ITemporaryStorageTextHandle

nit: would it be useful for ITemporaryStorageTextHandle and ITemporaryStorageStreamHandle to derive from a common interface?

It almost seems like there could just be a single

ITemporaryStorageHandle interface that returned TStorage from the Read* methods

Maybe? I just don't see it buying much . This isn't a system we've needed to generalize. It handles byte streams and encoded strings effectively. I'm fine keeping it specific to those two needs.

ToddGrun · 2024-04-25T02:25:18Z

...Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageStreamHandle.cs

+        {
+            // Return a read-only view of the underlying buffer to prevent users from overwriting or directly
+            // disposing the backing storage.
+            return new MemoryStream(streamCopy.GetBuffer(), 0, (int)streamCopy.Length, writable: false);


new MemoryStream

Does this method not get called on many of the TrivialStorageStreamHandle instance created? If it's called on most/all of them, it seems like this could just be stored in the field.

If the earlier comment about combining the interfaces into ITemporaryStorageHandle were used, and if the caller would just pass in an appopriate read only stream, then the stream/text versions of these classes could be combined too.

I'm preserving the behavior from before. I'm genuinely not sure if we have a single scenario exercising this code in the real world, so I'd prefer to leave the semantics as is for safety.

ToddGrun · 2024-04-25T02:34:27Z

src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs

        => "Roslyn Temp Storage " + size.ToString() + " " + Guid.NewGuid().ToString("N");

-    public sealed class TemporaryTextStorage : ITemporaryTextStorageInternal, ITemporaryStorageWithName
+    public sealed class TemporaryStorageTextHandle(


TemporaryStorageTextHandle

nit: consider moving to it's own file

ToddGrun · 2024-04-25T02:45:31Z

...ces/Core/Portable/TemporaryStorage/TemporaryStorageService.DirectMemoryAccessStreamReader.cs

+        public override int Peek()
+        {
+            if (_position >= _end)
+            {


{

nit: might as well format the document as part of the file creation

ToddGrun

CyrusNajmabadi · 2024-04-26T01:59:02Z

@jasonmalinowski for review when you get back.

CyrusNajmabadi added 7 commits April 24, 2024 10:36

Add initial stubs and trivial impl

0abbdda

Flesh out

be63421

Flesh out

f1faa2c

refactory

fe66325

Simplify

bc51702

Delete helper type

c8badaa

Delete helper type

1b9cc7c

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 24, 2024

CyrusNajmabadi commented Apr 24, 2024

View reviewed changes

Simplify

e4a2ecc

CyrusNajmabadi commented Apr 24, 2024

View reviewed changes

CyrusNajmabadi added 2 commits April 24, 2024 12:00

Simplify

61e33a1

Merge branch 'main' into textStorage2

4a765d7

CyrusNajmabadi marked this pull request as ready for review April 24, 2024 20:04

CyrusNajmabadi requested a review from a team as a code owner April 24, 2024 20:04

CyrusNajmabadi commented Apr 24, 2024

View reviewed changes

CyrusNajmabadi requested a review from ToddGrun April 24, 2024 20:32

ToddGrun reviewed Apr 24, 2024

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageService.cs Show resolved Hide resolved

CyrusNajmabadi added 2 commits April 24, 2024 17:02

Merge remote-tracking branch 'upstream/main' into textStorage2

895c658

Add docs

7d27e26

CyrusNajmabadi requested a review from ToddGrun April 25, 2024 00:05

ToddGrun reviewed Apr 25, 2024

View reviewed changes

CyrusNajmabadi requested a review from ToddGrun April 25, 2024 15:17

ToddGrun approved these changes Apr 25, 2024

View reviewed changes

CyrusNajmabadi merged commit f60d444 into dotnet:main Apr 25, 2024

CyrusNajmabadi deleted the textStorage2 branch April 25, 2024 16:37

dotnet-policy-service bot added this to the Next milestone Apr 25, 2024

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024


		namespace Microsoft.CodeAnalysis.Host;

		internal interface ITemporaryStorageTextHandle

Conversation

CyrusNajmabadi commented Apr 24, 2024

Uh oh!

CyrusNajmabadi Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ToddGrun left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi Apr 24, 2024 •

edited

Loading