Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 351258

Summary: performance problems with ExportedPackageImpl constructor
Product: [Eclipse Project] Equinox Reporter: Sangjin Lee <sjlee0>
Component: FrameworkAssignee: Thomas Watson <tjwatson>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: jgawor, jwross, tjwatson
Version: unspecified   
Target Milestone: Juno M1   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 351438    
Attachments:
Description Flags
proposed patch tjwatson: iplog+

Description Sangjin Lee CLA 2011-07-06 01:11:51 EDT 
Build Identifier: 3.7

The ExportedPackageImpl constructor, which gets called numerous times in most of the package admin service, is one of the top performance hotspots.

During the constructor call, Version.toString() is called. Even after Version.toString() is optimized (324331), this remains a hotspot, as it seems that Version objects don't seem to be reused, thus negating the benefit of the Version.toString() optimization.

The specVersion computation doesn't appear to be a central operation in ExportedPackageImpl, and yet each constructor call incurs a call to Version.toString().

I tried out a change where specVersion is lazily computed (computed only if getSpecVersion() is called), and that leads to a *substantial* performance improvement. Would it be possible to have this fixed and optimized that way? I can provide a patch if needed. Thanks!


Reproducible: Always
Comment 1 John Ross CLA 2011-07-06 09:07:10 EDT 
It would be interesting to see the before and after metrics from your performance tests.
Comment 2 Thomas Watson CLA 2011-07-06 09:13:10 EDT 
Hi, thanks for investigating this issue.  Any patch would be appreciated!  I recommend getting rid of the "specVersion" field on ExportedPackageImpl altogether and simply calling exportedPackage.getVersion().toString() when needed.  BTW, there are currently null checks for the return value of exportedPackage.getVersion(), this is unnecessary since getVersion() must not return null.  All packages have a version, the default version is 0.0.0 if not specified.
Comment 3 Sangjin Lee CLA 2011-07-06 15:45:07 EDT 
The performance improvement is pretty significant.

One of our test cases is the first page performance of a typical-sized JSP in our app running in Geronimo 3.0 for example. The first page invocation triggers JSP compilation, which in turn exercises Class.forName() and ultimately the package admin service as part of the dynamic classloading. We see roughly ~ 30% performance improvement with that use case.

Other use cases like OBR resolution and stuff like that also benefit from this, but to a varying degree.
Comment 4 Sangjin Lee CLA 2011-07-06 16:02:57 EDT 
Created attachment 199208 [details]
proposed patch

I attached the proposed patch. I'm not sure if the null check in toString() is still needed, but left it in. Also, I repeated the method call of exportedPackage.getVersion().toString() in toString() instead of calling getSpecificationVersion(), as it looks like getSpecificationVersion() is deprecated. Thanks!
Comment 5 Thomas Watson CLA 2011-07-07 08:39:24 EDT 
Thanks Sangjin!  I think we can safely remove the null check in toString() but other than that the patch looks good.  I will remove the null check before releasing.
Comment 7 Thomas Watson CLA 2011-07-07 17:53:55 EDT 
Comment on attachment 199208 [details]
proposed patch

thanks for the contribution!