Bug 222399 - Regression in several Model performance tests
Summary: Regression in several Model performance tests
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.4 M6   Edit
Assignee: Jerome Lanneluc CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance, test
Depends on:
Blocks:
 
Reported: 2008-03-12 08:52 EDT by Frederic Fusier CLA
Modified: 2008-03-26 06:36 EDT (History)
2 users (show)

See Also:


Attachments
Proposed fix (2.86 KB, patch)
2008-03-12 14:08 EDT, Jerome Lanneluc CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frederic Fusier CLA 2008-03-12 08:52:59 EDT
I20080305-1100 performance results show a regression in following test of FullSourceWorkspaceModelTests:
 - testProjectFindKnownSecondaryType():           between +4.5% and -13.1%
 - testProjectFindUnknownType():                  between +2.3% and -15.8%
 - testProjectFindUnknownTypeAfterSetClasspath(): between -0.6% and -12.4%

Even if results are positive on one box for the two first tests, all others are frankly negative and local JDT/Core perf results confirm the regression on these two tests:
 - testProjectFindKnownSecondaryType(): -11.51%
 - testProjectFindUnknownType():        -14.07%

The local results show that this regression appeared between v_839 and v_840.
The releng results confirm this as this regression appeared between N20080224-0010 and I20080226-1155 builds.
Comment 1 Olivier Thomann CLA 2008-03-12 09:23:38 EDT
The problem is that these two tests don't take a significant amount of time. They are both below 1s. So any little change (even a GC not performed at the right time) might cause a regression.
Comment 2 Frederic Fusier CLA 2008-03-12 09:56:44 EDT
Many performance tests have a duration below 1s, so it could not be a valid explanation for a regression. If it was only GC or something randomly happening, then our local tests with smoothed results would not get the same regression...
Comment 3 Olivier Thomann CLA 2008-03-12 10:12:32 EDT
When the performance test suite was originally created, the number 1 rule for adding new performance test was to have a duration above 1s. I didn't invent it.
Comment 4 Frederic Fusier CLA 2008-03-12 10:42:50 EDT
It was not 1s but 100ms...

Extracted from the "Performance Tests How To" accessible from 'Platform-releng FAQ' - http://wiki.eclipse.org/index.php/Platform-releng-faq#How_do_I_set_up_performance_tests.3F):

"...
As a rule of thumb the measured code should take at least 100ms on the target machine in order for the measurements to be relevant."

That's why I consider this test relevant.
Comment 5 Jerome Lanneluc CLA 2008-03-12 14:08:59 EDT
Created attachment 92349 [details]
Proposed fix

The performance regression seems to come from a change in PackageFragmentRoot#resource() which is heavily used (by PackageFragmentRoot#hashCode() for instance). This regression was introduced with the fix to bug 182537.

I also noticed that the call in NameLookup to PackageFragmentRoot#getKind() was modifying the timestamp of the element in the Java model cache. This is not needed during name lookup. So I changed NameLookup to use a version of getKind() that doesn't modify the timestamp.
Comment 6 Jerome Lanneluc CLA 2008-03-12 14:11:06 EDT
The proposed fix has been committed to HEAD so that tonight's nightly build performance run will use this fix. 

I will close the bug if the results are back to normal with this fix.
Comment 7 Jerome Lanneluc CLA 2008-03-17 06:47:55 EDT
No performance tests have run on nightly builds recently (due to a problem with the network on releng side). However our local performance run show that the regression is no longer there. So marking this bug as fixed.

Released for 3.4M6.
Comment 8 David Audel CLA 2008-03-26 06:36:04 EDT
Verified for 3.4M6.