Skip to content

Immutable MiniYaml#20965

Merged
PunkPun merged 1 commit into
OpenRA:bleedfrom
RoosterDragon:mini-yaml-immutable
Aug 7, 2023
Merged

Immutable MiniYaml#20965
PunkPun merged 1 commit into
OpenRA:bleedfrom
RoosterDragon:mini-yaml-immutable

Conversation

@RoosterDragon
Copy link
Copy Markdown
Member

This changeset is motivated by a simple concept - get rid of the MiniYaml.Clone and MiniYamlNode.Clone methods to avoid deep copying yaml trees during merging. MiniYaml becoming immutable allows the merge function to reuse existing yaml trees rather than cloning them, saving on memory and improving merge performance. On initial loading the YAML for all maps is processed, so this provides a small reduction in initial loading time.

The rest of the changeset is dealing with the change in the exposed API surface. Some With* helper methods are introduced to allow creating new YAML from existing YAML. Areas of code that generated small amounts of YAML are able to transition directly to the immutable model without too much ceremony. Some use cases are far less ergonomic even with these helper methods and so a MiniYamlBuilder is introduced to retain mutable creation functionality. This allows those areas to continue to use the old mutable structures. The main users are the update rules and linting capabilities.


CPU measures the percentage of time during the initial loading screen this is spent in the MiniYaml.Merge method. Memory measures the amount of inclusive memory (i.e. their memory plus everything they reference, recursively) retained by MiniYamlNodes once loading has completed.

Mod CPU Bleed CPU PR Memory Bleed Memory PR
RA 8.3% 4.6% 9.8 MB 7.8 MB
TD 5.3% 3.6% 10.1 MB 8.0 MB
TS 4.4% 2.8% 10.0 MB 7.9 MB
D2K 3.6% 2.3% 9.7 MB 7.7 MB

If you have a 3 second load time, this saves 0.04s to 0.11s. Users with many custom maps will see more improvement.

Copy link
Copy Markdown
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but given the size of the PR I'd be surprised if it doesn't contain at least one regression.

I'd suggest to wait for prep before merging it, though it would be best to merge asap as it refactors a core part of the engine and the PR will need to be constantly rebased

@Mailaender
Copy link
Copy Markdown
Member

Requires a rebase.

Copy link
Copy Markdown
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crash when trying to convert a map, the map can be downloaded from the description of #20991

Unhandled exception. System.InvalidCastException: Unable to cast object of type 'OpenRA.MiniYamlNode[]' to type 'System.Collections.Generic.List1[OpenRA.MiniYamlNode]'. at OpenRA.MapField.Serialize(Map map, List1 nodes) in /OpenRA/OpenRA.Game/Map/Map.cs:line 134

Copy link
Copy Markdown
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another crash when trying to create a new map

Exception of type System.ArgumentException: Object of type 'System.Collections.Immutable.ImmutableArray1[OpenRA.MiniYamlNode]' cannot be converted to type 'System.Collections.Generic.List1[OpenRA.MiniYamlNode]'.
at System.RuntimeType.TryChangeType(Object value, Binder binder, CultureInfo culture, Boolean needsSpecialCast)
at System.Reflection.MethodBase.CheckArguments(StackAllocedArguments& stackArgs, ReadOnlySpan1 parameters, Binder binder, BindingFlags invokeAttr, CultureInfo culture, Signature sig) at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) at OpenRA.ObjectCreator.CreateUsingArgs(ConstructorInfo ctor, Dictionary2 args) in /OpenRA/OpenRA.Game/ObjectCreator.cs:line 150

This changeset is motivated by a simple concept - get rid of the MiniYaml.Clone and MiniYamlNode.Clone methods to avoid deep copying yaml trees during merging. MiniYaml becoming immutable allows the merge function to reuse existing yaml trees rather than cloning them, saving on memory and improving merge performance. On initial loading the YAML for all maps is processed, so this provides a small reduction in initial loading time.

The rest of the changeset is dealing with the change in the exposed API surface. Some With* helper methods are introduced to allow creating new YAML from existing YAML. Areas of code that generated small amounts of YAML are able to transition directly to the immutable model without too much ceremony. Some use cases are far less ergonomic even with these helper methods and so a MiniYamlBuilder is introduced to retain mutable creation functionality. This allows those areas to continue to use the old mutable structures. The main users are the update rules and linting capabilities.
@RoosterDragon
Copy link
Copy Markdown
Member Author

Thanks. Have pushed a fix for both of these. Both of them involved reflection. I have checked for other uses of Type.NodeList (involved the first failure) and ObjectCreator.UseCtor (involved in the second failure which is trying to create a SaveMapLogic instance) and didn't find any additional sites which would need adjusting.

@PunkPun PunkPun merged commit 949ba58 into OpenRA:bleed Aug 7, 2023
@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Aug 7, 2023

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants