Created attachment 142628 [details] [review]
change proposals

Hi, thanks for the suggestion and patch, but:

I do not like things like
> #if 1

and even more

> if (i < ncol ) {
>    puts("FOR PERFORMACE AND USER FREANDLY REASON THIS SHOULD NEVER BE HERE");
>    abort();
> }

the part in mir_from_cols adds unnecessary requirements on underlying data, from my point of view, I do not like such requirements. So omit the #if and choose one or the other. There is always history of changes, so no need to keep this kind of code in sources. Thinking of this part, you can keep the old for, just do something like:
  if (!checked[0] && strcmp (...) == 0) {
     checked[0] = TRUE; ...
  } else if (!checked[1] && strcmp (...) == 0) {
   ...
  }

with a nice comment why is the checked array good to have, and as there is a pattern, I can see this having covered under some define with three parameters, index to 'checked' array, column name and an expression what to do when that column name matches.

I do not trust in things like this, not talking about readability for others:
> if ( *((unsigned short *)url_string) == (unsigned short)('/' << 8 | '/' ) ) {

I see you are using incorrect comments, and do not follow evolution coding style. I do not see why you commented out lines marked with
> //this is current null due container creation method

You are creating variables in the middle of a code block. We are trying to be compliant event with older C compilers.

Please follow patch basics and coding style guidelines described at
http://projects.gnome.org/evolution/patch.shtml
and update your patch accordingly. The best for the latest git master, as this change can be committed only there, but not necessary.

(In reply to comment #2)
Hi Milan

Thanks for your time wich you need spend on my poor code. 

> Hi, thanks for the suggestion and patch, but:
> 
> I do not like things like
> > #if 1
> 
> and even more
> 
> > if (i < ncol ) {
> >    puts("FOR PERFORMACE AND USER FREANDLY REASON THIS SHOULD NEVER BE HERE");
> >    abort();
> > }
> 
> the part in mir_from_cols adds unnecessary requirements on underlying data,
> from my point of view, I do not like such requirements. So omit the #if and
> choose one or the other. There is always history of changes, so no need to keep
> this kind of code in sources. Thinking of this part, you can keep the old for,
> just do something like:
>   if (!checked[0] && strcmp (...) == 0) {
>      checked[0] = TRUE; ...
>   } else if (!checked[1] && strcmp (...) == 0) {
>    ...
>   }
> 
> with a nice comment why is the checked array good to have, and as there is a
> pattern, I can see this having covered under some define with three parameters,
> index to 'checked' array, column name and an expression what to do when that
> column name matches.

I full agree with you that leaving old code/designs in "#if 0/1" blocks in code is primary reasons to droping programes to hell... but I don't send you final solution only my quick (and ugly) fix. I leve old code for maitainer of this code to help him/her better understand problem. I also don't like my ugly fix but currently it work for me and I have own dutes in work wich also take time :(.
 
I suspect that person who wite prev. code write "if" blocks ~ in same sequence which I saw in debuger then suspect it don't change often. My fix based on this same only with this difference that I stop check the same stings again and agin. The problem is that I "truly" hardoced sequence which is danger since I don't have time to study entire Evo. design and don't know if sequence of input strings array (parsed by those "if" code) may change or not. And for practical reason of my private solution I use simple "abort" for secnarion when something change in Evo and code will generate automaticaly "abort". Which tell me to fix the code. 

I know that Maintainer can't agree for such apporoch but I simple have no time for writting beter algorithm which will secure all case. I deside touch this code because profiler show very ugly numbers here but as I say my code is only proposals for proper fix.

Thanks for making my primay work applicatinons better and better...
Rafal

With respect of those 'abort' calls, when you would need to switch between older and newer evolution, then those might trigger, which is not as that great.

Fixing those things shouldn't be as that hard, I believe, or do you think to use your patch as a start point, and I should choose only parts I like and change them as I believe they are "the best"?

Created attachment 146061 [details]
partial eds test change

I tried only few parts from your patch, as you can see in this attachment, and improved it slightly (I thought I'm improving it, actually). I was measuring time spent in a call of camel_folder_summary_load_from_db and I do not see any significant difference, I see only a difference of 200 ms on my 68091 messages folder. The total time is ~2.8 seconds spent in the function with this patch. Which means the slowness doesn't come from here, and these parts can be safely omitted from any change.

I look on your rest changes soon.

For a comparison, the same folder, just without the patch applied, I see 2740 ms, which is caused also by the fact that some columns are missing in a query for the mir (We are trying to read something what we do not fetch from db, what a shame.)

I tried to look through your changes, but your patch is not applicable to 2.29 sources (actual master), thus I only tried a function camel_url_decode, where I felt an issue. And it is really not working correctly with incorrect inputs, as for input "-%%4--end%20-" it returns "- -", which should be "-%%4--end -" as it does the actual implementation.

With respect of sorting summary infos, I do not think it's necessary to have a function in the base class, when only IMAP has anything to do with that, furthermore, why not using a GHashTable instead of a binary search? As your changes are adding unnecessary complexity to the code base, from my point of view.

Please update the patch to master and drop changes which doesn't seem to improve anything (or tell me how better I should test the changes, as it's possible I did my measurement incorrectly). Note that I'm still against changes in camel-url.c. Coding style issues and similar from the comment #2 still stands as well.

note of some effort to improve performance by changing cache size of SQLite within bug #595389. I guess this can be replaced by the other?

k. I'm closing this bug then, assuming that the patch doesn't make it.