In addition to persisting the elfie index as a txt file, persist it as a binary file #76509

ToddGrun · 2024-12-18T23:18:40Z

This will allow subsequent loads of this index to be read from the binary file (which ScottLo indicated should be more allocation friendly).

Speedometer results (from commit 1) came back good. Filtered CPU/Allocations with IncPaths "elfie"

Old CPU:

Old Allocations:

New CPU:

New Allocations:

…s a binary file, allowing it to on sunsequent loads to be read from the binary file (which ScottLo indicated should be more allocation friendly). Created as a draft PR to get speedometer results.

CyrusNajmabadi · 2024-12-18T23:27:47Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

@@ -289,17 +289,45 @@ private async Task<TimeSpan> DownloadFullDatabaseAsync(FileInfo databaseFileInfo
            // Write the file out to disk so we'll have it the next time we launch VS.  Do this
            // after we set the in-memory instance so we at least have something to search while
            // we're waiting to write.
-            await WriteDatabaseFileAsync(databaseFileInfo, bytes, cancellationToken).ConfigureAwait(false);
+            await WriteDatabaseTextFileAsync(databaseFileInfo, bytes, cancellationToken).ConfigureAwait(false);
+            await WriteDatabaseBinaryFileAsync(database, databaseFileInfo, cancellationToken).ConfigureAwait(false);


there's a part of me that is concerned about torn writes. specifically that one of these succeeds and the other does not. leading to an inconsistent state.

I think it's OK for the txt file to be written out and the binary file to not be. The subsequent VS session will just read from the txt file and again try to persist out the binary file.

what about the reverse? or do these functions fail instead of continuing on? if this is the case, doc that it's ok to fail to write this file.

Looking at this a little closer, this is tricky and I don't know if it works the way it's intended currently. The RepeatIOAsync doesn't appear to throw if it fails the maximum number of times. I'm hesitant to change it as maybe I'm not understanding the exception flow through that call.

After a bit more consideration, I think it's ok if the bin file is persisted and the txt file isn't. The next VS run will not find the txt file and will go through the DownloadFullDatabaseAsync path in UpdateDatabaseInBackgroundWorkerAsync and use the newly downloaded file to populate the db rather than the bin file.

CyrusNajmabadi · 2024-12-18T23:36:38Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

        {
            LogInfo("Writing database file");

+            await WriteDatabaseFileAsync(databaseFileInfo, new ArraySegment<byte>(bytes), cancellationToken).ConfigureAwait(false);


curiosu what arraysegment is buying us here.

Originally had this as a ReadOnlySpan, but that caused the following difficulties:

IOService.WriteAndFlushAllBytes needed to have different code for the FileStream.Write calls as netstandard2.0 doesn't have the ReadOnlySpan overload.

The unit tests hit some issues because they are written in VB and using an It.IsAny specifying a ReadOnlySpan was causing compiler errors and me to bang my head against the wall.

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

CyrusNajmabadi · 2024-12-18T23:38:24Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

-            var databaseBytes = _service._ioService.ReadAllBytes(databaseFileInfo.FullName);
+            var databaseBinaryFileInfo = GetBinaryFileInfo(databaseFileInfo);
+            var isBinary = _service._ioService.Exists(databaseBinaryFileInfo);
+            var databaseBytes = _service._ioService.ReadAllBytes(isBinary ? databaseBinaryFileInfo.FullName : databaseFileInfo.FullName);


there is a race-condition here between checking existence and then reading. could we just attempt to ReadAllBytes of hte binary file, and fall over it we get null there?

CyrusNajmabadi · 2024-12-18T23:39:01Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

+            var delayUntilUpdate = await ProcessPatchXElementAsync(
+                databaseFileInfo,
+                element,
+                getDatabaseBytes: () => isBinary ? _service._ioService.ReadAllBytes(databaseFileInfo.FullName) : databaseBytes,


took me a second. can you doc.

CyrusNajmabadi · 2024-12-18T23:39:51Z

stepping back. if binary is supported (i wasn't aware of that), why not do 100% of our work in the binary space?

ToddGrun · 2024-12-19T03:15:41Z

Commit 1 test insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/599405

Get rid of a potential race and instead use an exception via ReadAllBytes.

ToddGrun · 2024-12-19T04:25:56Z

stepping back. if binary is supported (i wasn't aware of that), why not do 100% of our work in the binary space?

I didn't think that was an option because of the patching code. Although I didn't really try to fully understand that code,. Could patching work without having the original bytes? Manual debugging isn't going through the patch code, but manually inspecting the code leads me to NativePatching.ApplyPatch, which looks like the patchBytes is based upon the original bytes.

CyrusNajmabadi · 2024-12-19T04:49:49Z

Could patching work without having the original bytes?

Probably not. Good point :)

ToddGrun · 2024-12-19T14:37:23Z

@CyrusNajmabadi -- Does the logging in this code provide value? It doesn't write it to telemetry, but just stores the logged string in a static LinkedList. If it doesn't provide value for you, I'd be inclined to just remove it as it's a bit of clutter.

CyrusNajmabadi · 2024-12-19T16:29:02Z

Does the logging in this code provide value? It doesn't write it to telemetry,

It should get written into vs' activity log file.

And it has definitely provided value as to why things aren't working. We used it to track down a cdn issue one time :-)

ToddGrun · 2024-12-19T17:15:55Z

It should get written into vs' activity log file.

It's not obvious to me how it's doing that if so

In reply to: 2554945112

CyrusNajmabadi · 2024-12-19T17:44:29Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

+
+            // Obtain the underlying array from the memory stream. If for some reason this isn't available,
+            // fall back to reading the stream into a new byte array.
+            if (!memoryStream.TryGetBuffer(out var arraySegmentBuffer))


outside of this PR. but i think we could benefit from thsi pattern in a few more places. like in the sqlite system. def takea look.

I didn't find anything with my quick search, was there a particular class you had in mind?

i was thinking about Sqlite Accessor.WriteStream which which works with streams and does some funky stuff with SQLitePersistentStorage.GetBytes to try to an array to write to to then write to the DB.

That method is a very light allocator in the csharp editing speedometer (less than 1 MB allocated). I took a look at the code and it wasn't straight forward to me how to map this pattern to it, so I'll avoid this change unless you push back.

CyrusNajmabadi · 2024-12-19T17:44:58Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

+            if (!memoryStream.TryGetBuffer(out var arraySegmentBuffer))
+            {
+                memoryStream.Position = 0;
+                arraySegmentBuffer = new ArraySegment<byte>(writer.BaseStream.ReadAllBytes());


why are you doing writer.BaseStream, instead of just memoryStream here? is there a difference between the two? if so, please doc.

ReadAllBytes is defined on a Stream. To get to that from the BinaryWriter, you need to get the BaseStream. (will add doc)

chatted offline, and I misread the request. Updating in commit 4

CyrusNajmabadi · 2024-12-19T17:46:38Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

-            // (intentionally not wrapped in IOUtilities.  If this throws we want to restart).
-            var databaseBytes = _service._ioService.ReadAllBytes(databaseFileInfo.FullName);
+
+            var databaseBinaryFileInfo = GetBinaryFileInfo(databaseFileInfo);


i feel like i want tshi section extracted into a method or local function.

CyrusNajmabadi · 2024-12-19T17:47:54Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs


            // We have patch data.  Apply it to our current database bytes to produce the new
            // database.
            LogInfo("Applying patch");
            var finalBytes = _service._patchService.ApplyPatch(databaseBytes, patchBytes);
            LogInfo($"Applying patch completed. finalBytes.Length={finalBytes.Length}");

-            CreateAndSetInMemoryDatabase(finalBytes);
+            database = CreateAndSetInMemoryDatabase(finalBytes, isBinary: false);


i think you should doc why you're passing isBinary:false here.

Use memoryStream directly instead of writer.BaseStream Break some code into a separate method

CyrusNajmabadi · 2024-12-19T18:46:07Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

-                // (intentionally not wrapped in IOUtilities.  If this throws we want to restart).
-                databaseBytes = _service._ioService.ReadAllBytes(databaseFileInfo.FullName);
-            }
+            var databaseBytes = GetDatabaseBytes(databaseFileInfo, out var isBinary);


Suggested change

var databaseBytes = GetDatabaseBytes(databaseFileInfo, out var isBinary);

var (databaseBytes, isBinary) = GetDatabaseBytes(databaseFileInfo);

CyrusNajmabadi · 2024-12-19T18:46:45Z

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs

+            byte[] GetDatabaseBytes(FileInfo databaseFileInfo, out bool isBinary)
+            {
+                var databaseBinaryFileInfo = GetBinaryFileInfo(databaseFileInfo);
+                isBinary = true;
+
+                try
+                {
+                    // First attempt to read from the binary file. If that fails, fall back to the text file.
+                    return _service._ioService.ReadAllBytes(databaseBinaryFileInfo.FullName);
+                }
+                catch (Exception e) when (IOUtilities.IsNormalIOException(e))
+                {
+                }
+
+                isBinary = false;
+
+                // (intentionally not wrapped in IOUtilities.  If this throws we want to restart).
+                return _service._ioService.ReadAllBytes(databaseFileInfo.FullName);
+            }


Suggested change

byte[] GetDatabaseBytes(FileInfo databaseFileInfo, out bool isBinary)

{

var databaseBinaryFileInfo = GetBinaryFileInfo(databaseFileInfo);

isBinary = true;

try

{

// First attempt to read from the binary file. If that fails, fall back to the text file.

return _service._ioService.ReadAllBytes(databaseBinaryFileInfo.FullName);

}

catch (Exception e) when (IOUtilities.IsNormalIOException(e))

{

}

isBinary = false;

// (intentionally not wrapped in IOUtilities. If this throws we want to restart).

return _service._ioService.ReadAllBytes(databaseFileInfo.FullName);

}

(byte[] dataBytes, bool isBinary) GetDatabaseBytes(FileInfo databaseFileInfo)

{

var databaseBinaryFileInfo = GetBinaryFileInfo(databaseFileInfo);

try

{

// First attempt to read from the binary file. If that fails, fall back to the text file.

return (_service._ioService.ReadAllBytes(databaseBinaryFileInfo.FullName), isBinary: true);

}

catch (Exception e) when (IOUtilities.IsNormalIOException(e))

{

}

// (intentionally not wrapped in IOUtilities. If this throws we want to restart).

return (_service._ioService.ReadAllBytes(databaseFileInfo.FullName), isBinary: false);

}

…and out parameter

In addition to persisting the elfie index as a txt file, persist it a…

2f4cc7a

…s a binary file, allowing it to on sunsequent loads to be read from the binary file (which ScottLo indicated should be more allocation friendly). Created as a draft PR to get speedometer results.

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

dotnet-policy-service bot added the VSCode label Dec 18, 2024

CyrusNajmabadi reviewed Dec 18, 2024

View reviewed changes

src/Features/Core/Portable/SymbolSearch/Windows/SymbolSearchUpdateEngine.Update.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 18, 2024

View reviewed changes

build-analysis bot mentioned this pull request Dec 19, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Add some comments

11fc402

Get rid of a potential race and instead use an exception via ReadAllBytes.

Add comments indicating Write failure behavior.

2d56327

ToddGrun marked this pull request as ready for review December 19, 2024 17:11

ToddGrun requested a review from a team as a code owner December 19, 2024 17:11

CyrusNajmabadi reviewed Dec 19, 2024

View reviewed changes

CyrusNajmabadi approved these changes Dec 19, 2024

View reviewed changes

Comment

da2b9dc

Use memoryStream directly instead of writer.BaseStream Break some code into a separate method

CyrusNajmabadi reviewed Dec 19, 2024

View reviewed changes

return tuple instead of splitting method output between return value …

e589fea

…and out parameter

ToddGrun merged commit 9a27ef7 into dotnet:main Dec 19, 2024
25 checks passed

dotnet-policy-service bot added this to the Next milestone Dec 19, 2024

ToddGrun mentioned this pull request Dec 20, 2024

ItemTree PartialArray allocations hit LOH microsoft/elfie-arriba#258

Open

dibarbet modified the milestones: Next, 17.13 P3 Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In addition to persisting the elfie index as a txt file, persist it as a binary file #76509

In addition to persisting the elfie index as a txt file, persist it as a binary file #76509

ToddGrun commented Dec 18, 2024 •

edited

Loading

CyrusNajmabadi Dec 18, 2024

ToddGrun Dec 18, 2024

CyrusNajmabadi Dec 18, 2024

ToddGrun Dec 19, 2024

ToddGrun Dec 19, 2024

CyrusNajmabadi Dec 18, 2024

ToddGrun Dec 19, 2024

CyrusNajmabadi Dec 18, 2024

CyrusNajmabadi Dec 18, 2024

CyrusNajmabadi commented Dec 18, 2024

ToddGrun commented Dec 19, 2024

ToddGrun commented Dec 19, 2024 •

edited

Loading

CyrusNajmabadi commented Dec 19, 2024

ToddGrun commented Dec 19, 2024

CyrusNajmabadi commented Dec 19, 2024

ToddGrun commented Dec 19, 2024

CyrusNajmabadi Dec 19, 2024

ToddGrun Dec 20, 2024

CyrusNajmabadi Dec 20, 2024

ToddGrun Dec 20, 2024

CyrusNajmabadi Dec 19, 2024

ToddGrun Dec 19, 2024 •

edited

Loading

ToddGrun Dec 19, 2024

CyrusNajmabadi Dec 19, 2024

CyrusNajmabadi Dec 19, 2024

CyrusNajmabadi Dec 19, 2024

CyrusNajmabadi Dec 19, 2024

	var databaseBytes = GetDatabaseBytes(databaseFileInfo, out var isBinary);
	var (databaseBytes, isBinary) = GetDatabaseBytes(databaseFileInfo);

In addition to persisting the elfie index as a txt file, persist it as a binary file #76509

In addition to persisting the elfie index as a txt file, persist it as a binary file #76509

Conversation

ToddGrun commented Dec 18, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 18, 2024

ToddGrun commented Dec 19, 2024

ToddGrun commented Dec 19, 2024 • edited Loading

CyrusNajmabadi commented Dec 19, 2024

ToddGrun commented Dec 19, 2024

CyrusNajmabadi commented Dec 19, 2024

ToddGrun commented Dec 19, 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

ToddGrun Dec 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun commented Dec 18, 2024 •

edited

Loading

ToddGrun commented Dec 19, 2024 •

edited

Loading

ToddGrun Dec 19, 2024 •

edited

Loading