Common - Optimize CBA_fnc_compatibleMagazines

[johnb432]

When merged this pull request will:

• Changed to hashmaps for caching.
• Generally faster performance.
• In the same idea of Joint Rails - Optimize CBA_fnc_compatibleItems #1558.

[commy2]

If it's cached anyway, why bother dicking around with dictionaries when building up the arrays the first time? This getOrDefaultCall syntax is an abomination.

I like the namespace -> HashMap part though.

Any benchmark before after?

[johnb432]

If it's cached anyway, why bother dicking around with dictionaries when building up the arrays the first time?

Because it's faster. You don't have to use pushBackUnique, which has to verify the entire array before pushing or not. Same goes for arrayIntersect on an array on itself. With the hashmap if something is already in it, you overwrite it and it doesn't matter.

This getOrDefaultCall syntax is an abomination.

The reason I used that is because it's faster.

Any benchmark before after?

I will run some tonight and report the numbers, but from the limited testing I ran it's always faster.

[johnb432]

I like the namespace -> HashMap part though.

Something I have just thought of: I could make the hashmap stored in uiNamespace, instead of refreshing the cache on mission start.
Is this something that would be wanted?

[commy2]

No, that can be abused.

[johnb432]

I tested with "arifle_AK12_F" in a big modset.

In a first test, I ran only the code from CBA_fnc_compatibleMagazines that gets the magazines (from Line 38 to Line 61 and from Line 37 to Line 65). I wanted to see how fast the functions would get the magazines if the weapon wasn't cached. This is what I got:

["arifle_AK12_F"] call _oldCode; // 0.70 ms
["arifle_AK12_F"] call _newCode; // 0.20 ms

I reran the first test, but with this time to get all magazines. However, these do call CBA_fnc_compatibleMagazines, which was the new code (which means that the old benefitted from the new hashmap caching during this specific test).

["arifle_AK12_F", true] call _oldCode; // 0.25 ms
["arifle_AK12_F", true] call _newCode; // 0.0475 ms

So for getting magazines the first time, the new code is significantly faster.

For conventional usage:

"arifle_AK12_F" call CBA_fnc_compatibleMagazines // Old, 0.0070 ms
"arifle_AK12_F" call CBA_fnc_compatibleMagazines // New, 0.0067 ms

["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // Old, 0.0073 ms
["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // New, 0.0070 ms

// Much smaller modset:
"arifle_AK12_F" call CBA_fnc_compatibleMagazines // Old, 0.0051 ms
"arifle_AK12_F" call CBA_fnc_compatibleMagazines // New, 0.0046 ms

["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // Old, 0.0053 ms
["arifle_AK12_F", true] call CBA_fnc_compatibleMagazines // New, 0.0048 ms

---

No, that can be abused.

How so? How is having a uiNamespace variable more prone to abuse than a missionNamespace variable?

[commy2]

How so? How is having a uiNamespace variable more prone to abuse than a missionNamespace variable?

You can put compileFinal'd function in there before it is populated by an actual value and therefore can cause scripts to fail unexpectedly.

missionNamespace is cleared at mission start, therefore can't carry over values from, e.g. the editor.

[commy2]

So for getting magazines the first time, the new code is significantly faster.

That's irrelevant then though. .X milliseconds once per mission is a meaningless improvement. Readability trumps premature optimization here.

The relevant result is pretty much the same. ~5-10% improvement. Not worth it.

[johnb432]

You can put compileFinal'd function in there before it is populated by an actual value and therefore can cause scripts to fail unexpectedly.

missionNamespace is cleared at mission start, therefore can't carry over values from, e.g. the editor.

If you have a mod loaded that loads anything in cba_jr_namespace (missionNamespace or uiNamespace) besides CBA, then imo it's for the better that the game will throw errors, as you will be forced to unload that mod in order to play. The only reason I could see why cba_jr_namespace would have anything assigned to it from another mod is malicious intent. The variable name is too specific to be accidentally used by another mod.

If there is a will, there is a way. If people want to break something, they will be able to. You could just as easily break cba_jr_namespace in missionNamespace as in uiNamespace.

---

That's irrelevant then though. .X milliseconds once per mission is a meaningless improvement.

The relevant result is pretty much the same. ~5-10% improvement. Not worth it.

How is it irrevelant, meaningless or not worth it? Arma runs like a piece of shit. Any improvement should be a welcome one.

Readability trumps premature optimization here.

I seriously fail to see how the readability of the code has been compromised. Just when comparing the difference side by side you can see it's not fundamentally different.
I'm sorry to hear you don't like getOrDefaultCall, but it's meant for this exact purpose. The code could not be cleaner imo: The engine does a lot internally.

"Premature" really rubs me the wrong way. What is the premature part?
The only premature part I see is that the first entry of CBA_fnc_compatibleMagazines' return is not the first magazine listed in the magazines array of a weapon, which is considered to be the default by e.g. the arsenal. So from that point of view I could understand the premature comment.
However, you didn't point that out, so I imagine you are referring to something else.

[commy2]

then imo it's for the better that the game will throw errors

Errors can be ignored. The failed script however may give the player an ingame advantage. Yes, malicious intend.

I have reported bugs of ui namespace abuse to BI multiple times in the past, and they have all fixed them. Even got me two extra copies of APEX as reward, because some of these enabled arbitrary code execution.

It's hard to utilize ui namespace without introducing exploits. It's not worth it to work around these exploits when the potential gain is a laughable < 0.001 seconds per mission start per weapon. That's what makes any of this premature.

"Premature" really rubs me the wrong way. What is the premature part?
The only premature part I see is that the first entry of CBA_fnc_compatibleMagazines' return is not the first magazine listed in the magazines array of a weapon, which is considered to be the default by e.g. the arsenal. So from that point of view I could understand the premature comment.

"Premature optimization" is a phrase coined by Donald Knuth.

The cache itself is properly utilized optimization. It made using the function essentially free. I still like the part where you rewrite it to use the native HashMap instead of the custom CBA namespace. I have nothing against modernizing old code.

[johnb432]

I still like the part where you rewrite it to use the native HashMap instead of the custom CBA namespace. I have nothing against modernizing old code.

Ultimately, what does this mean? What changes are going to be accepted besides switching GVAR(magNamespace) to be a hashmap? What about getOrDefaultCall - yay or nay?

[commy2]

What changes are going to be accepted besides switching GVAR(magNamespace) to be a hashmap?

If it were up to me, that + the function header fixes.

What about getOrDefaultCall - yay or nay?

nay, not worth it

[johnb432]

If it were up to me, that + the function header fixes.

If nobody else says anything in the near future, I'll go with what you said.

[johnb432]

If it were up to me, that + the function header fixes.

Seeing as nobody has said anything, going ahead with your suggestion.

[commy2]

lgtm