Community
Participate
Working Groups
The fix for bug 289057 was intentionally curbed to be as safe as possible for M6. However, there are some other identified places where some more improvements could be done. This bug intends to track the complementary work to do in this area...
Created attachment 164936 [details] Proposed patch When the results are being added while looking up the category table, they are searched to see if it is already present. However, it is not necessary in some cases. I have tried to take advantage of this conditions in this attached patch I am working on the performance numbers - I will update once I have them.
Here are the numbers running on the windows machine with the eclipse full source and the project including the offending jar. FullSourceWorkspaceSearchTests#testSearchAllTypeNames takes 29.36s(Patch)vs 109.2s(Head) FullSourceWorkspaceSearchTests#testNewSearchAllTypeNames takes 3.03s(Patch) vs 10.84s(Head) FullSourceWorkspaceSearchTests#testSearchAllTypeNameMatches takes 3.31s(Patch) vs 11.52s(Head) All the other search tests look more or the same. I am still running the tests on the eclipse full source without the offending jar.
For FullSourceWorkspaceSearchTests on the eclipse full source, there is a small improvement ~1-2% of performance compared to the Head with this patch.
Frederic, Can you review this patch?
(In reply to comment #4) > Frederic, Can you review this patch? I do not understand the logic of checkBeforeAdd boolean? In some places it's outside the for loop and on others it's inside. It seems to me to have a big impact on the behavior of the called method addQueryResult(...). Could you explain your change a little bit more by adding comments on the new lines? Another cosmetic remark: EntryResult result = null; if (checkBeforeAdd) result = (EntryResult) results.get(word); Is usually written as: EntryResult result = checkBeforeAdd ? (EntryResult) results.get(word) : null; I know that some code guidance disapprove the usage of the ternary conditional operator, but we prefer to use it in JDT/Core code when the expressions are not too long to save 2 extra lines of code... TIA
Frederic, Thanks for the comments. I will put a better comment and use the ternary conditional. The results hashtable will not contain the document name for the first category lookup. Hence, the get() is un-necessary. Subsequent category lookups may have the document names and thus the boolean checkBeforeAdd should be true for the subsequent category lookups. So, this flag is getting set at the end of the category lookup for loop. This is not set if the first category doesn't have any results. Hope this explains.
Created attachment 165230 [details] Revised Patch Added a better comment and used the terenary expression as per Frederic's comment. Also changed the condition for the initialization of checkBeforeAdd. That could be initialized to false instead of depending on the presence of the memoryIndex.
(In reply to comment #7) > Created an attachment (id=165230) [details] > Revised Patch > > Added a better comment and used the terenary expression as per Frederic's > comment. Also changed the condition for the initialization of checkBeforeAdd. > That could be initialized to false instead of depending on the presence of the > memoryIndex. OK, I get it now... Hence, I do not think the local variable checkBeforeAdd is necessary... You can replace it with <code>i > 0</code> and that will do the trick with, I think, a more obvious meaning. Then, I would also change the parameter name as multipleCategory or something like that, which would also means more the reason of the check than the fact you'll do it (which is redundant while reading the code...)
Created attachment 165368 [details] Revised Patch Changed the variable name to prevResults -- I couldn't think of a better name. Also changed the way the prevResults is being set. Now it is set to (results != null).
Comment on attachment 165368 [details] Revised Patch Patch looks good to me. The name is not ideal but let's take it until we find another better one :-)
(In reply to comment #10) > (From update of attachment 165368 [details]) > Patch looks good to me. The name is not ideal but let's take it until we find > another better one :-) Released for 3.6M7 in HEAD stream.
Reopened as performance results of I20100425-2000 build do not show any improvement for this test... :-(
Satyam, Please try to fix the problem with the released patch. If we get a fix for tomorrow, we might release it for M7.
Satyam, please make sure get Frédéric to check the patch on his local performance machine before it is released. Thanks.
(In reply to comment #12) > Reopened as performance results of I20100425-2000 build do not show any > improvement for this test... :-( Frederic, This bug was actually meant to primarily improve the testcase provided in bug 289057. Further to that, I could see performance improvement of testNewSearchAllTypeNames compared to M6. Yes, it doesn't improve the performance of FullSourceWorkspaceTypeHierarchyTests#testPerfAllTypes(), which is anyways being tracked by bug 306170. Am I missing anything?
(In reply to comment #15) > (In reply to comment #12) > > Reopened as performance results of I20100425-2000 build do not show any > > improvement for this test... :-( > Frederic, > This bug was actually meant to primarily improve the testcase provided in bug > 289057. Further to that, I could see performance improvement of > testNewSearchAllTypeNames compared to M6. > Yes, it doesn't improve the performance of > FullSourceWorkspaceTypeHierarchyTests#testPerfAllTypes(), which is anyways > being tracked by bug 306170. > Am I missing anything? No, you're right... I'm definitely sorry, it seems I mixed these two bugs :-S
Hence, verified for 3.6M7 looking at I20100425-2000 performance results
