small refactoring in globals.cpp

[frithjofh]

hi everybody,

took a look into file globals.cpp and I prose some refactoring of three functions:

- general code clean up
- improved readability
- simplification of logic
- removed some duplication
- speed up measured to be about 14% due, I suppose, to more cache locality (all is relative: on my machine, with my test case,... )

regards

frithjof

[oBFusCATed]

GetVectorFromString should not call wxArrayString, because there is an additional copy!

[frithjofh]

because first I have a const wxArrayString and then return a std::vector ?

my thoughts were: in the original function I have two vector's, one for the values in the function and the other as a copy during the return.

Now I have one wxArrayString and because the function is so "short" the unnamed object which is returned gets the benefit of RVO. so there are at most the same number of containers, at best even one less. the original function cannot count on RVO imho.

but maybe I get something wrong here...

thx for he feedback

[frithjofh]

nah... I think you are right... there is an extra copy of the array... damm

[frithjofh]

so maybe make a template function out of it in order to not textually duplicate the code... what do you think?

never mind... i am too tired to think strait

[oBFusCATed]

A template is fine, but generally doesn't make much difference.

[frithjofh]

not for the size of executable, but to avoid the duplicate

[frithjofh]

factory recall !

found a strange error in my patch. sorry, haven't tested it enough, I think.

will post new patch these days

[frithjofh]

here the new version of the patch.

discovered, that assign() for wxArrayString does something else than it should. it appends the given values instead of replacing the contents of the wxArrayString...

if anyone is interested, here my measuring in msec for the functions returning an array/vector from a string:

item count -	current arry -	new array -	current std::vec -	new std::vec
512	1	0	1	0
1.200	7	0	7	0
2.500	24	0	26	1
5.000	93	2	106	3
10.000	402	2	395	6

regards

[frithjofh]

reworked the next function: EscapeSpaces

speed up in msec:

words in text |	current version |	new version
32	0	0
64	1	0
128	6	0
256	28	0
512	97	0
1024	388	0
2048	1570	0
4096	6264	1

removed the comment about speeding this function up.

some minor changes

regards

[Alpha]

Have not tested, but it looks like GetStringFromArray() will have problems with an empty array.

[oBFusCATed]

Why are you doing 3 iterations on the array in GetArrayFromString?
Why are you doing additional copy of the whole array?
And why are you doing Mid(a, npos-something) (hint happens when the array doesn't have a separator at the end)?

[oBFusCATed]

Code (diff) Select Expand


-    if (!ret.IsEmpty() && ret[0] != _T('"') && ret[0] != _T('\''))
+    if (str.IsEmpty() || str.StartsWith(_T("\"")) || str.StartsWith(_T("\'")))


Why is this change needed? Why have you replaced the operator[] with startswith?

[frithjofh]

@alpha

thx, you are right. I will add a test for empty array.

[frithjofh]

@obfuscated

doing three separate iterations on the array results in the code running much faster due to more cache locality. my measurements confirmed the speedup. the original version continuously jumped from one context to another. putting each action in a separate loop is just faster.

for instance, the original version repeated the test for the value of trimspaces in each iteration. now it is done only once.

same for the third iteration of removing empty entries.

the part of the extra whole copy I don't understand. do you mean in the return statement? if so, that would happen in the original version too. or is there another extra copy that slipped me? are you looking at the last version of the patch? the one before had the problem of the extra copy in the std:vector version of the function, but I already changed that.

and if there would be an extra copy, wouldn't the gained speed justify it?

as to your last point: hit and sunk. my functions have a problem with the case where there is no separator at the end. it just omits the part after the last separator. I will correct it, slipped me, thx for the find.