DavidVine / amx-util-library

Various useful NetLinx include files and modules
MIT License
34 stars 11 forks source link

dictionary.axi - performance improvements #6

Closed si-hb closed 3 years ago

si-hb commented 3 years ago

Arguably splitting hairs considering the miniscule memory involved here, however if I may suggest two performance improvements:

dictionaryRemove:

Considering that the dictionary is an unordered list, remove the loop to shift the entire array on deletion and simply move the key pair at the end of the list to the item position being removed.

dict.keyVals[idx].key = dict.keyVals[length_array(dict.keyVals)].key;
dict.keyVals[idx].val = dict.keyVals[length_array(dict.keyVals)].val;
set_length_array(dict.keyVals,length_array(dict.keyVals)-1);

The clear memory operation can also be removed, as reducing the array length size by one is sufficient. Similarly with dictionaryclear, the loop is not required.

dictionaryAdd:

The repetitive call to dictionaryGetIndex is costly:

if(dictionaryGetIndex(dict,key)) { 
     dict.keyVals[dictionaryGetIndex(dict,key)].val = val;

Gain a 2x performance improvement with the following:

stack_var integer idx;
idx=dictionaryGetIndex(dict,key);
if(idx)) { 
     dict.keyVals[idx].val = val;
DavidVine commented 3 years ago

Hi @si-hb ,

Thanks for your suggestions!

With regards to the suggestion for dictionaryRemove, I like the suggestion to simply move the key pair at the end of the dictionary to the item position being removed rather than looping through. You're right, it's more efficient given it's an unordered list and we don't really care if the order changes. I'll make this change and push an update. I'll be keeping the clear memory operation though as I like to have empty values outside the length bounds of the array, even though only the values within the length of the array are used when referencing the array as a whole. I'll need to do a check for when the key/val pair to be removed is the last item in the dictionary.

I'm going keep dictionaryClear the way it currently is for the same reason I won't be removing the clear memory operations from dictionaryRemove. This is a personal preference and unfortunately my OCD outweighs my desire for increased efficiency :)

I'll make that change to dictionaryAdd. Nice catch!

-Dave

DavidVine commented 3 years ago

Changes have been made and pushed to master. Thanks again!

si-hb commented 3 years ago

My pleasure @DavidVine. Thank you for making these axi's available.

Ditto re OCD. :)