[GEN][ZH] Reduce cost of GameMessage::getCommandTypeAsAsciiString by 70%

[Caball009]

• Closes task: GameMessage::getCommandTypeAsAsciiString looks inefficient #838

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/Common/MessageStream.cpp

Lines 231 to 250 in 9527b54

	AsciiString GameMessage::getCommandTypeAsAsciiString(GameMessage::Type t)
	{
	#define CHECK_IF(x) if (t == x) { return #x; }
	AsciiString commandName = "UnknownMessage";
	if (t >= GameMessage::MSG_COUNT)
	{
	commandName = "Invalid command";
	}
	CHECK_IF(MSG_INVALID)
	CHECK_IF(MSG_FRAME_TICK)
	CHECK_IF(MSG_RAW_MOUSE_BEGIN)
	CHECK_IF(MSG_RAW_MOUSE_POSITION)
	CHECK_IF(MSG_RAW_MOUSE_LEFT_BUTTON_DOWN)
	CHECK_IF(MSG_RAW_MOUSE_LEFT_DOUBLE_CLICK)
	CHECK_IF(MSG_RAW_MOUSE_LEFT_BUTTON_UP)
	CHECK_IF(MSG_RAW_MOUSE_LEFT_CLICK)
	CHECK_IF(MSG_RAW_MOUSE_LEFT_DRAG)
	CHECK_IF(MSG_RAW_MOUSE_MIDDLE_BUTTON_DOWN)
	CHECK_IF(MSG_RAW_MOUSE_MIDDLE_DOUBLE_CLICK)
	CHECK_IF(MSG_RAW_MOUSE_MIDDLE_BUTTON_UP)

This PR changes a long list of if statements in getCommandTypeAsAsciiString into a single switch statement for slightly better performance.

TODO

• Replicate in Generals
• Measure performance

[Caball009]

Reviewing will probably be easier per commit. I tried to keep the commits as clean as possible; the first two only change the macro name and nothing else.

[xezon]

Can you measure the cost of this function please?

[Caball009]

Can you measure the cost of this function please?

Measure what exactly? In isolation or in-game? It's going to be an improvement either way, but very minimal. This code is only used in debug mode (well, DEBUG_LOGGING) so not even modern compilers will turn the original code to a switch statement; they do it in release mode, though.

[xezon]

Looks correct.

[aliendroid1]

is the point of the original code to be a stupid hack to check the enum against the enum name?

[aliendroid1]

If it does not yeild a measurable performance boost then its not a change worth making now especially if it only affects debug builds and can be done better after dropping vc6.

But there are other reasons to consider updating the code. The original code does looks very hacky, relying on some weird macro tricks.

We should investigate what is really happening here and see if it can be done with more standard syntax.

[aliendroid1]

Actually the implementation is correct but since we have access to modern text editors and can edit multiple lines at the same time, I think its better to skip the macro altogether and just use switch statements directly. Having to write the enum name twice is really not much work with find replace and regex

[Caball009]

Actually the implementation is correct but since we have access to modern text editors and can edit multiple lines at the same time, I think its better to skip the macro altogether and just use switch statements directly. Having to write the enum name twice is really not much work with find replace and regex

I'm hardly a fan of macros but think it's fine here for the time being. It's simple and it's local to the function so it doesn't leak through to other code.

Having to write the enum name twice is really not much work with find replace and regex

This would introduce the potential for typos or string literals that don't match (changed) enum values, that's just bad for maintainability imo.

[aliendroid1]

its inherently problamatic code so unfortunately there isn't really a good local fix especially with just c++98 so I think your implementation is a decent enough improvement for the time being

[xezon]

Another oprimization we can do here is return const char* instead of AsciiString, because it is not necessary to allocate here.

[Caball009]

Right. I can change that. That does require a change to getContentsAsAsciiString as well. Should I just do the change or remove the entire function? (#1259 (comment))

[xezon]

We can remove the functions if you like.

[Caball009]

Changed return type to const char* and removed function AsciiString getContentsAsAsciiString.

[xezon]

How about we remove this function as well? It is unused. Or did I look wrong?

Other than that, the function name is getCommandTypeAsAsciiString but it now returns const char*. It would have to be named getCommandTypeName then.

[Caball009]

That one is still used. I updated the names and return types of both functions, as well as the function calls for getCommandAsAsciiString.

[xezon]

Performance measurement

{
	std::srand(111);
	int64_t begin = GetTickCount64();
	GameMessage::Type t = GameMessage::Type(std::rand() % GameMessage::MSG_COUNT);
	//GameMessage::Type t = GameMessage::Type(GameMessage::MSG_COUNT / 2);
	for (Int i = 0; i < 10000000; ++i)
	{
		AsciiString str = GameMessage::getCommandTypeAsAsciiString(t);
		DEBUG_ASSERTCRASH(!str.isEmpty());
	}

	int64_t time = GetTickCount64() - begin;
	DEBUG_LOG(("time old %lld ms", time));
}
{
	std::srand(111);
	int64_t begin = GetTickCount64();
	GameMessage::Type t = GameMessage::Type(std::rand() % GameMessage::MSG_COUNT);
	//GameMessage::Type t = GameMessage::Type(GameMessage::MSG_COUNT / 2);
	for (Int i = 0; i < 10000000; ++i)
	{
		AsciiString str = GameMessage::getCommandTypeAsString(t);
		DEBUG_ASSERTCRASH(!str.isEmpty());
	}

	int64_t time = GetTickCount64() - begin;
	DEBUG_LOG(("time new %lld ms", time));
}

Result

time old 8390 ms
time new 2375 ms

Over 70% faster.