Hi Peter,
Thanks for the review feedback.
>
> NamedPackage p = packages.get(name);
> if (p instanceof Package) {
> return (Package) p;
> }
>
> return (Package)packages.compute((n, p) -> {
> // define Package object if it is not yet defined or replace it if it is a
> NamedPackage
> return (p instanceof Package) ? p : NamedPackage.toPackage(n, m);
> });
>
> See, no private ClassLoader.toPackage(String name, NamedPackage p) needed.
>
I like this suggestion that allows me to remove this private method. I have
this patch:
http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/jigsaw-m3/webrev-03-16/
One thing to note is that I don’t view definePackage returning Package object
is performance critical.
java.lang.Package is legacy and from my scan of its usage, it's mainly used for
getting the package name. The versioning info is more for tracing/debugging
use. Package objects has not been defined for every packages. Not to mention
its assumption on class loader hierarchy and packages are not splitted among
JAR files.
In jake, Alex and I rewrote the java.lang.Package class spec. Also fixed
several design issue of Package. I’d recommend any use of
Class.getPackage().getName() be replaced with Class::getPackageName which is
more performant if performance is important to them. Package annotation
(package-info) is the main useful info to be obtained from Package object.
Otherwise I am not aware anything in Package class needed to be performant.
>
> If you also care for constant lambda, this could be optimized even further,
> but for the price of more complex code:
Simple clean code as above is good for this case.
> On Mar 16, 2016, at 10:30 AM, Peter Levart <peter.lev...@gmail.com> wrote:
>
> In java.lang.ClassLoader:
>
> ...the package-private definePackage(String name, Module m) is OK to use a
> single packages.compute(...) call performance-wise since it is pre-screened
> with packages.get() in public getDefinedPackage(String name) method. But
> there's also a package-private packages() method (a basis for public methods
> getPackages() and getDefinedPackages()) that constructs a Stream<Package> of
> defined Packages which unnecessarily calls definePackage() for each value of
> packages map:
>
> Stream<Package> packages() {
> return packages.values().stream()
> .map(p -> definePackage(p.packageName(), p.module()));
> }
>
>
> It would be nice performance-wise to avoid calling definePackage if the value
> is already a Package:
>
> Stream<Package> packages() {
> return packages.values().stream()
> .map(p -> p instanceof Package
> ? (Package) p
> : definePackage(p.packageName(), p.module()));
> }
>
As explained above, getting a Package object is not performance critical. I’ll
keep this in mind for other performance critical code.
thanks
Mandy
