Community
Participate
Working Groups
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
It would be interesting to see the before and after metrics from your performance tests.
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.
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.
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!
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.
Fixed in git master: http://git.eclipse.org/c/equinox/rt.equinox.framework.git/commit/?id=6e8d1f0f7061a652f25da8f4fbf5cd4c666fb250
Comment on attachment 199208 [details] proposed patch thanks for the contribution!
