Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FieldLoader, use dictionary for GetValue to make it twice as fast. #19205

Merged
merged 1 commit into from Mar 17, 2021

Conversation

anvilvapre
Copy link
Contributor

Replace the very large if-then-else clause with a dictionary lookup making it 52% faster.

The total time spent in the GetValue until right after game launch or after game replay is half (189720686/91038340).

Improves game loading speed and there is some gain in-game.

(I had expected the difference to be far less. Which may have to do with the order of entries in the if-then-else clause, or many calls to typeof.)

@pchote
Copy link
Member

pchote commented Mar 1, 2021

@teinarss can you comment on how this fits with your work to pull FieldLoader out into its own package?

@pchote pchote requested a review from teinarss March 1, 2021 23:58
OpenRA.Game/FieldLoader.cs Outdated Show resolved Hide resolved
OpenRA.Game/FieldLoader.cs Outdated Show resolved Hide resolved
OpenRA.Game/FieldLoader.cs Outdated Show resolved Hide resolved
OpenRA.Game/FieldLoader.cs Show resolved Hide resolved
OpenRA.Game/FieldLoader.cs Outdated Show resolved Hide resolved
@teinarss
Copy link
Contributor

teinarss commented Mar 2, 2021

@teinarss can you comment on how this fits with your work to pull FieldLoader out into its own package?

I'm rewriting the parser from scratch so this is fine!

Copy link
Member

@pchote pchote left a comment

Choose a reason for hiding this comment

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

LGTM overall, i've verified that this is just moving around the existing parsing code without appearing to change any behaviour. For this reason, i'm okay with leaving any style nits that existed in the original code (several unnecessary sets of braces) to a followup PR.

Just one question/request:

OpenRA.Game/FieldLoader.cs Outdated Show resolved Hide resolved
pchote
pchote previously approved these changes Mar 13, 2021
Comment on lines 83 to 112
TypeParsers.Add(typeof(int), ParseInt);
TypeParsers.Add(typeof(ushort), ParseUShort);
TypeParsers.Add(typeof(long), ParseLong);
TypeParsers.Add(typeof(float), ParseFloat);
TypeParsers.Add(typeof(decimal), ParseDecimal);
TypeParsers.Add(typeof(string), ParseString);
TypeParsers.Add(typeof(Color), ParseColor);
TypeParsers.Add(typeof(Hotkey), ParseHotkey);
TypeParsers.Add(typeof(HotkeyReference), ParseHotkeyReference);
TypeParsers.Add(typeof(WDist), ParseWDist);
TypeParsers.Add(typeof(WVec), ParseWVec);
TypeParsers.Add(typeof(WVec[]), ParseWVecArray);
TypeParsers.Add(typeof(WPos), ParseWPos);
TypeParsers.Add(typeof(WAngle), ParseWAngle);
TypeParsers.Add(typeof(WRot), ParseWRot);
TypeParsers.Add(typeof(CPos), ParseCPos);
TypeParsers.Add(typeof(CVec), ParseCVec);
TypeParsers.Add(typeof(CVec[]), ParseCVecArray);
TypeParsers.Add(typeof(BooleanExpression), ParseBooleanExpression);
TypeParsers.Add(typeof(IntegerExpression), ParseIntegerExpression);
TypeParsers.Add(typeof(Enum), ParseEnum);
TypeParsers.Add(typeof(bool), ParseBool);
TypeParsers.Add(typeof(int2[]), ParseInt2Array);
TypeParsers.Add(typeof(Size), ParseSize);
TypeParsers.Add(typeof(int2), ParseInt2);
TypeParsers.Add(typeof(float2), ParseFloat2);
TypeParsers.Add(typeof(float3), ParseFloat3);
TypeParsers.Add(typeof(Rectangle), ParseRectangle);
TypeParsers.Add(typeof(DateTime), ParseDateTime);
TypeParsers.TrimExcess();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use this syntax here? And on GenericTypeParsers below?

			TypeParsers = new Dictionary<Type, Func<string, Type, string, MemberInfo, object>>
			{
				{ typeof(int), ParseInt },
				{ typeof(ushort), ParseUShort },
				{ typeof(long), ParseLong },
				{ typeof(float), ParseFloat },
				{ typeof(decimal), ParseDecimal },
				{ typeof(string), ParseString },
				{ typeof(Color), ParseColor },
				{ typeof(Hotkey), ParseHotkey },
				{ typeof(HotkeyReference), ParseHotkeyReference },
				{ typeof(WDist), ParseWDist },
				{ typeof(WVec), ParseWVec },
				{ typeof(WVec[]), ParseWVecArray },
				{ typeof(WPos), ParseWPos },
				{ typeof(WAngle), ParseWAngle },
				{ typeof(WRot), ParseWRot },
				{ typeof(CPos), ParseCPos },
				{ typeof(CVec), ParseCVec },
				{ typeof(CVec[]), ParseCVecArray },
				{ typeof(BooleanExpression), ParseBooleanExpression },
				{ typeof(IntegerExpression), ParseIntegerExpression },
				{ typeof(Enum), ParseEnum },
				{ typeof(bool), ParseBool },
				{ typeof(int2[]), ParseInt2Array },
				{ typeof(Size), ParseSize },
				{ typeof(int2), ParseInt2 },
				{ typeof(float2), ParseFloat2 },
				{ typeof(float3), ParseFloat3 },
				{ typeof(Rectangle), ParseRectangle },
				{ typeof(DateTime), ParseDateTime }
			};

@teinarss teinarss merged commit be22493 into OpenRA:bleed Mar 17, 2021
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.

None yet

4 participants