Refactor `SerializationResourceManager` to use `Dictionary` instead of `HashTable` by elachlan · Pull Request #8332 · dotnet/winforms

elachlan · 2022-12-06T01:27:21Z

Refactored ResourceCodeDomSerializer.SerializationResourceManager to replace usages of Hashtable.
Refactored ResXResourceReader to use Dictionary instead of ListDictionary

Related: #8143

Microsoft Reviewers: Open in CodeFlow

…shTable

elachlan · 2022-12-06T01:29:03Z

src/System.Windows.Forms/src/System/Resources/ResXResourceReader.cs

            _isReaderDirty = true;
            EnsureResData();
-            return _resData.GetEnumerator();
+            return ((IDictionary)_resData).GetEnumerator();


This part was required, which might make the whole thing pointless if there is a performance cost for the cast?

Ultimately it would be nice if these were updated to be properly typed, but that is a breaking change.

IDictionary is one of the last implemented interfaces, so performance won't be so great.
https://www.thomasclaudiushuber.com/2020/03/05/c-the-order-of-interfaces-is-important-for-casting-performance/

@JeremyKuhne I'd love your input on this. The move to generics might be a performance regression here.

Casting is not going to measure in a scenario like this. While there is a difference, it is incredibly small. A few nanoseconds are not going to matter in something that won't run more than a few times.

dreddy-work · 2022-12-06T22:00:52Z

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

-                }
-
-                return null;
+                Dictionary<string, object> ht = GetResourceSet(culture);


nullable declaration?

dreddy-work · 2022-12-06T22:00:54Z

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

                    {
                        // This is for backwards compatibility and also for the case when our reader/writer don't support metadata.  We must merge the original enumeration data in here or  else existing design time properties won't show up.  That would be really bad for things like Localizable.
-                        Hashtable it = GetResourceSet(CultureInfo.InvariantCulture);
+                        Dictionary<string, object> it = GetResourceSet(CultureInfo.InvariantCulture);


...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

dreddy-work · 2022-12-06T22:05:41Z

@elachlan, I see partial nullability in these files. There are multiple instances that needs to be declared as nullable.

elachlan · 2022-12-07T03:00:27Z

@elachlan, I see partial nullability in these files. There are multiple instances that needs to be declared as nullable.

@dreddy-work System.Windows.Forms.Design does not have <Nullable>enable</Nullable> in its project file. So for this issue I'd like to avoid the nullability changes for now and come back to it in another PR.

Edit: I've created an issue for it at #8342

dreddy-work · 2022-12-07T17:49:30Z

@elachlan , seems some left out causing build failures : src\System.Windows.Forms.Design\src\System\ComponentModel\Design\Serialization\ResourceCodeDomSerializer.SerializationResourceManager.cs(458,40): error CS8632: (NETCORE_ENGINEERING_TELEMETRY=Build) The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.

elachlan · 2022-12-07T22:27:46Z

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

@@ -874,23 +803,23 @@ public string SetValue(IDesignerSerializationManager manager, ExpressionContext
                {


@dreddy-work The for loop here has a super weird definition. There are two of these types of for loops within this code file. What would you suggest to do here?

Loop is trying to find unique name that wasn't used yet. We could simplify it to do-while for readability and should be able to remove warning. I do not see any other technical advantages though. Let's convert to do-while?

I think I can write this into a Dictionary<string, int> and use the baseName as the key. Therefore keeping track of the count. I'll push through once I work out the proper logic.

@dreddy-work done! Both should be easier to understand now I think.

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

dreddy-work · 2022-12-08T00:40:51Z

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs

+                if (resourceSet is not null && resourceSet.TryGetValue(name, out object parentValue))
                {
-                    return CompareValue.Different;
+                    return !parentValue.Equals(value) || parentValue is null ? CompareValue.Different : CompareValue.Same;


what if both parentValue and value is null?

@elachlan , did you run any tests here? This is core for resource serialization and like to do some unit testing performed with this change and record results before we merge it.

I did not do any manual testing, but did run the Design Unit Tests.

The ordering seemed important here. Because previously it was checking if both were equal before checking for null. Meaning that its designed to allow both to be null for Same to be returned. So I replicated that logic here.

can you try a simple application with multiple sets of controls and localize the application?

I have no idea how to do this. You mean as a part of one of the test projects?

No, by replacing binaries locally on your machine and create .NET 8.0 targeted application. Here are instructions on private testing in case you have not used them before: https://github.com/dotnet/winforms/blob/main/docs/testing.md

I was playing around with the tests, the code is only hit in SerializationResourceManager when the PrimitiveCodeDomSerializer is serializing a string with a char count > 200. This isn't tested for.

I tried testing this scenario and writing a separate test class for ResourceCodeDomSerializer. Both resulted in exceptions due to either a missing session. If I manually created a session, then the exception would be on SerializationResourceManager.Writer with a missing ResourceWriter service.

If I could work out how to add a writer service in the test, then I can write tests for it.

good to know that. Logically, change is no-op here. Lets take it in after addressing rest of the comments.

Refactor SerializationResourceManager to use Dictionary instead of Ha…

4367629

…shTable

elachlan requested a review from a team as a code owner December 6, 2022 01:27

ghost assigned elachlan Dec 6, 2022

elachlan commented Dec 6, 2022

View reviewed changes

dreddy-work reviewed Dec 6, 2022

View reviewed changes

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs Outdated Show resolved Hide resolved

dreddy-work reviewed Dec 6, 2022

View reviewed changes

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs Outdated Show resolved Hide resolved

dreddy-work added the waiting-author-feedback The team requires more information from the author label Dec 6, 2022

ghost removed the waiting-author-feedback The team requires more information from the author label Dec 7, 2022

invert some if statements and rename touched short named variables

c478495

dreddy-work added the waiting-author-feedback The team requires more information from the author label Dec 7, 2022

elachlan commented Dec 7, 2022

View reviewed changes

ghost removed the waiting-author-feedback The team requires more information from the author label Dec 7, 2022

Fix null annotation

991b7db

dreddy-work reviewed Dec 7, 2022

View reviewed changes

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs Outdated Show resolved Hide resolved

dreddy-work reviewed Dec 7, 2022

View reviewed changes

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs Show resolved Hide resolved

dreddy-work added the waiting-author-feedback The team requires more information from the author label Dec 7, 2022

ghost removed the waiting-author-feedback The team requires more information from the author label Dec 7, 2022

elachlan added 3 commits December 8, 2022 10:09

Refactor strange for loops

eb0104a

Changes from review

6dbb873

use pattern matching

0fbad45

dreddy-work reviewed Dec 8, 2022

View reviewed changes

...omponentModel/Design/Serialization/ResourceCodeDomSerializer.SerializationResourceManager.cs Outdated Show resolved Hide resolved

dreddy-work reviewed Dec 8, 2022

View reviewed changes

Remove redundant variable

a4d7272

dreddy-work enabled auto-merge (squash) December 8, 2022 18:03

dreddy-work approved these changes Dec 8, 2022

View reviewed changes

dreddy-work merged commit f0d0562 into dotnet:main Dec 8, 2022

ghost added this to the 8.0 Preview1 milestone Dec 8, 2022

elachlan deleted the SerializationResourceManager-HashTable branch December 8, 2022 21:33

ghost locked as resolved and limited conversation to collaborators Jan 8, 2023

		@@ -874,23 +803,23 @@ public string SetValue(IDesignerSerializationManager manager, ExpressionContext
		{

Conversation

elachlan commented Dec 6, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

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!

Uh oh!

dreddy-work commented Dec 6, 2022

Uh oh!

elachlan commented Dec 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dreddy-work commented Dec 7, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

elachlan commented Dec 6, 2022 •

edited by ghost

Loading

elachlan commented Dec 7, 2022 •

edited

Loading