Remove BinaryFormatter from GetSDKReferenceFiles by Forgind · Pull Request #6324 · dotnet/msbuild

Forgind · 2021-04-03T00:05:25Z

I think this is almost right, but TranslatorHelpers needs System.Collections.Concurrent on .NET 3.5, or I need to be able to translate a concurrent dictionary without translating it. (?) Any idea on how I can get around that?

KirillOsenkov · 2021-04-04T01:29:07Z

I would hardcode the translation logic for each type T that we need, and just write the count of key-value pairs, followed by each key and value separately.

Forgind · 2021-04-05T19:03:50Z

Can you explain why we need the counts? I haven't looked at how complete our test coverage of this task was, but it seems to pass whatever tests we previously had without it.

KirillOsenkov · 2021-04-05T20:11:57Z

I might be not understanding it well, but how are you going to read the keys and values if you don’t know how many there are?

Forgind · 2021-04-05T21:20:22Z

Looks like the answer is: there weren't tests. Makes it easy to pass them 😋

I was imagining something like looking for closing elements. In json, for instance, they have { "key1": "val1", "key2": "val2" }. You don't need a count because the opening and closing braces match. I don't know if there's an equivalent here, but I now think it just takes references and gets or sets them based on what sort of data you ask for, so since the dictionary would have started out full in serializing, it would serialize properly. Then, in deserialization, the dictionary would be null, and it would throw an exception. If I'd initialized the dictionary, it would still be empty and remain so. I added a count. I should probably add tests.

benvillalobos · 2021-04-09T21:01:54Z

src/Tasks/GetSDKReferenceFiles.cs

+        internal class SdkReferenceInfo
        {
            /// <summary>
            /// Constructor


While you're getting rid of useless summaries, might as well delete this one.

benvillalobos · 2021-04-09T21:03:59Z

src/Tasks/GetSDKReferenceFiles.cs

+            public ConcurrentDictionary<string, List<string>> DirectoryToFileList { get { return directoryToFileList; } }

            /// <summary>
            /// Hashset


nit: remove useless summary

This has the bonus of deleting a summary that isn't actually accurate. Turns out if a mapping isn't in the dictionary, it doesn't look for it; it just assumes it isn't there.

benvillalobos · 2021-04-09T21:04:09Z

src/Tasks/GetSDKReferenceFiles.cs

+            }

            /// <summary>
            /// Constructor


nit: remove useless summary

benvillalobos · 2021-04-09T21:07:00Z

src/Tasks/GetSDKReferenceFiles.cs

            /// A dictionary which maps a file path to a structure that contain some metadata information about that file.
            /// </summary>
-            public ConcurrentDictionary<string, SdkReferenceInfo> PathToReferenceMetadata { get; }
+            internal ConcurrentDictionary<string, SdkReferenceInfo> PathToReferenceMetadata { get { return pathToReferenceMetadata; } }


Why is this changed from public to internal, but other properties untouched?

I don't think this actually mattered, so I reverted it. See next comment for fiddling.

benvillalobos · 2021-04-09T21:25:05Z

src/Tasks/GetSDKReferenceFiles.cs

        /// This class represents the context information used by the background cache serialization thread.
        /// </summary>
-        private class SaveContext
+        internal class SaveContext


Why did this need to change?

I had to fiddle with a lot of these to make them accessible to the test class. Then I downgraded a couple because properties were more visible than their containing class, which isn't really good style, but the latter wasn't necessary. As long as something prevents it from being public/protected, I don't think it particularly matters.

Ah, I hadn't reviewed the test so I didn't see any need for it.

benvillalobos · 2021-04-09T21:25:06Z

src/Tasks/GetSDKReferenceFiles.cs

+            string[] keys = dictionary.Keys.ToArray();
+            for (int i = 0; i < count; i++)
+            {
+                if (i < keys.Length)


If i < keys.Length once, it won't ever change right? For better at-a-glance-understanding™, I'd suggest creating some bool translatingOut = i < keys.Length and using that bool. Or place a comment.

True. I put the if statement first, which should technically be more performant, too, though not in a particularly meaningful way.

KirillOsenkov · 2021-04-09T22:40:43Z

src/Tasks/GetSDKReferenceFiles.cs

+                for (int i = 0; i < count; i++)
+                {
+                    translator.Translate(ref keys[i]);
+                    T value = dictionary[keys[i]];


Can the dictionary change while the loop runs? If a value is removed, this may throw. Do you need to lock? Also perhaps its better to enumerate key value pairs to avoid looking every key up.

No—before this PR, there was only the property, and it had a getter but no setter, so it really shouldn't be a ConcurrentDictionary. It's been a ConcurrentDictionary since the initial code commit. Do you want me to change it to an ordinary dictionary?

up to you, if it feels safe

KirillOsenkov · 2021-04-09T22:41:20Z

src/Tasks/GetSDKReferenceFiles.cs

+            int count = dictionary.Count;
+            translator.Translate(ref count);
+            string[] keys = dictionary.Keys.ToArray();
+            if (keys.Length == 0)


is this effectively determining the translation direction (read/write)? If so, is there a more explicit way, perhaps asking the translator?

Looks like ITranslator has a TranslationDirection Mode we can take advantage of here.

rainersigwald

Looks ok as is but I'm pretty curious if we can avoid the ConcurrentDictionary/Dictionary stuff.

rainersigwald · 2021-04-27T16:15:20Z

src/Tasks/GetSDKReferenceFiles.cs

@@ -1246,63 +1218,79 @@ public SdkReferenceInfo(string fusionName, string imageRuntime, bool isWinMD, bo
        /// Structure that contains the on disk representation of the SDK in memory.
        /// </summary>
        /// <remarks>This is a serialization format. Do not change member naming.</remarks>


Suggested change

/// <remarks>This is a serialization format. Do not change member naming.</remarks>

Stale

rainersigwald · 2021-04-27T16:22:21Z

src/Tasks/GetSDKReferenceFiles.cs

                PopulateRedistDictionaryFromPaths(directoryToFileList, redistDirectories);

-                var cacheInfo = new SDKInfo(references, directoryToFileList, FileUtilities.GetPathsHash(directoriesToHash));
+                var cacheInfo = new SDKInfo(references.ToDictionary(kvp => kvp.Key, kvp => kvp.Value, StringComparer.OrdinalIgnoreCase), directoryToFileList.ToDictionary(kvp => kvp.Key, kvp => kvp.Value, StringComparer.OrdinalIgnoreCase), FileUtilities.GetPathsHash(directoriesToHash));


Is there a perf impact of this synchronization? Could you instead store an IDictionary in the SDKInfo fields and on creation it's ConcurrentDictionary/on deserialization it's a plain dictionary?

rainersigwald · 2021-04-27T16:23:43Z

src/Tasks/GetSDKReferenceFiles.cs

+            private Dictionary<string, SdkReferenceInfo> pathToReferenceMetadata;
+            private Dictionary<string, List<string>> directoryToFileList;
+            private int hash;


Our/runtime's standard naming convention would make these:

Suggested change

private Dictionary<string, SdkReferenceInfo> pathToReferenceMetadata;

private Dictionary<string, List<string>> directoryToFileList;

private int hash;

private Dictionary<string, SdkReferenceInfo> _pathToReferenceMetadata;

private Dictionary<string, List<string>> _directoryToFileList;

private int _hash;

benvillalobos

LGTM!

benvillalobos · 2021-05-14T19:32:09Z

src/Tasks/GetSDKReferenceFiles.cs

+                    info.IsWinMD = isWinmd;
+                }, count => new Dictionary<string, SdkReferenceInfo>(count, StringComparer.OrdinalIgnoreCase));
+
+                translator.TranslateDictionary(ref _directoryToFileList, (ITranslator t, ref string s) => t.Translate(ref s), (ITranslator t, ref List<string> fileList) =>


Funny that the duplicate translation worked because the same method handles input/output. Good thing it's not a huge concern and fairly easy to catch.

I wonder if a certain type of test would catch this? (not blocking on this thought)

Right, but it should work as long as it matches. It serialized the dictionary twice, then deserialized it twice. Setting it the second time overwrote the correct value with the correct value, so everything still worked.

That also means I think it would be very hard to write a test for it...the only two options I can think of are having some kind of counter on how many times we accessed/set each variable or having a check at the end for the total size of the serialized form. Neither is trivial, and it isn't a huge deal for something that gets hit as often as this.

Setting it the second time overwrote the correct value with the correct value, so everything still worked.

I see, I was thinking maybe there'd be duplicate data somehow. Agreed that it's fine as is.

Remove BF from GetSDKReferenceFiles

be6f54e

Private method for translating dictionaries

e68602d

Translate count

108c4c8

Add test and fix bugs

d090c3c

Forgind marked this pull request as ready for review April 9, 2021 16:43

Fix capitalization error

aae3133

benvillalobos changed the title ~~Remove BF from GetSDKReferenceFiles~~ Remove BinaryFormatter from GetSDKReferenceFiles Apr 9, 2021

benvillalobos reviewed Apr 9, 2021

View reviewed changes

PR feedback

855038d

KirillOsenkov reviewed Apr 9, 2021

View reviewed changes

Forgind added 3 commits April 12, 2021 07:45

PR feedback

46f4731

Switch to non-concurrent dictionary

0d1e723

ignore case

11e3f9b

marcpopMSFT linked an issue Apr 15, 2021 that may be closed by this pull request

BinaryFormatter Deprecation #6215

Closed

27 tasks

rainersigwald approved these changes Apr 27, 2021

View reviewed changes

Forgind added 2 commits April 27, 2021 17:04

Optimize dictionary passing

47e2a3f

Ignore case part 2

be38506

Forgind mentioned this pull request May 10, 2021

OutOfProcProjectInstanceBasedBuildDoesNotReloadFromDisk intermittently fails #3552

Closed

Remove extraneous dictionary translation

ee7cf13

benvillalobos approved these changes May 14, 2021

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label May 14, 2021

Forgind merged commit 9d41925 into dotnet:main May 24, 2021

Forgind deleted the remove-binaryformatter-from-state-files branch May 24, 2021 21:52

baronfel mentioned this pull request Jun 1, 2023

BinaryFormatter Deprecation #6215

Closed

27 tasks

Conversation

Forgind commented Apr 3, 2021

Uh oh!

KirillOsenkov commented Apr 4, 2021

Uh oh!

Forgind commented Apr 5, 2021

Uh oh!

KirillOsenkov commented Apr 5, 2021

Uh oh!

Forgind commented Apr 5, 2021

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!

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!

rainersigwald left a comment

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!

benvillalobos left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants