Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
On 2017-01-03 19:08, Karen Kinnear wrote: Claes, Thank you for all the good work you are doing identifying performance issues and making improvements. I strongly agree with Lois’ concerns. We looked at other cases in which we have an interface from java to the VM that need to pass fully qualified names, e.g. Class.forName(). The model used there might be useful to you as an alternative way to get the same performance benefits. see Class.c - which uses some existing libjava utilities to do the conversion. So this has the benefit of not creating extra java strings, while keeping the JVM interface consistent. It also reduces the risk of missing locations. Yes, I had forgotten to consider that we could do this kind of conversion in the JDK native code. This will mean the interface would need to change more than through this patch, though, as we'd want to pass a transformed char* rather than a jstring through. I'll accept the challenge, of course. :-) Thanks! /Claes
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
Claes, Thank you for all the good work you are doing identifying performance issues and making improvements. I strongly agree with Lois’ concerns. We looked at other cases in which we have an interface from java to the VM that need to pass fully qualified names, e.g. Class.forName(). The model used there might be useful to you as an alternative way to get the same performance benefits. see Class.c - which uses some existing libjava utilities to do the conversion. So this has the benefit of not creating extra java strings, while keeping the JVM interface consistent. It also reduces the risk of missing locations. We are happy to have further discussions offline if you like. If you think this is a more general issue, and want to revisit JVM APIs, then we can look at that in a future release, in which we will have time to consistently modify the model, and create common utilities. thanks, Karen > On Jan 3, 2017, at 11:44 AM, Claes Redestad wrote: > > > On 01/03/2017 04:56 PM, Lois Foltan wrote: >> >> Hi Claes, >> >> I have some concerns about this change in that it will break the precedence >> that currently the only internal form of names that the VM deals with are >> binary names as they appear in class files, where the periods (.) have been >> replaced by forward slashes (/). I think I would like to discuss this with >> the runtime team before you proceed. > > Hi Lois, > > I have anticipated some controversy on this one. :-) > >> >> As far as the actual changes go, if we do proceed with this, there are >> places in modules.cpp that have been missed. After line #535, #665, #749 >> (the replace should be moved before the verify of the package name occurs), >> #820. > > Well spotted. I've updated all places: > > Hotspot: http://cr.openjdk.java.net/~redestad/8171855/hotspot.02 > JDK: http://cr.openjdk.java.net/~redestad/8171855/jdk.01 > > get_module_by_package_name is only used by the whitebox API and it seems the > test using this was already using internal form. JVM_GetModuleByPackageName > appears to be unused. Could this be cleaned up? > > Thanks! > > /Claes > >> >> Thanks, >> Lois >> >>> >>> Thanks! >>> >>> /Claes >>> >> >
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
On 03/01/2017 16:44, Claes Redestad wrote: : get_module_by_package_name is only used by the whitebox API and it seems the test using this was already using internal form. JVM_GetModuleByPackageName appears to be unused. Could this be cleaned up? This may be a leftover from when we were working out the API changes for JVM TI, specifically what ended up as GetNamedModule to allow the callbacks for CLFH or class load get the Module object for a class that is being loaded. -Alan
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
On 01/03/2017 04:56 PM, Lois Foltan wrote: Hi Claes, I have some concerns about this change in that it will break the precedence that currently the only internal form of names that the VM deals with are binary names as they appear in class files, where the periods (.) have been replaced by forward slashes (/). I think I would like to discuss this with the runtime team before you proceed. Hi Lois, I have anticipated some controversy on this one. :-) As far as the actual changes go, if we do proceed with this, there are places in modules.cpp that have been missed. After line #535, #665, #749 (the replace should be moved before the verify of the package name occurs), #820. Well spotted. I've updated all places: Hotspot: http://cr.openjdk.java.net/~redestad/8171855/hotspot.02 JDK: http://cr.openjdk.java.net/~redestad/8171855/jdk.01 get_module_by_package_name is only used by the whitebox API and it seems the test using this was already using internal form. JVM_GetModuleByPackageName appears to be unused. Could this be cleaned up? Thanks! /Claes Thanks, Lois Thanks! /Claes
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
On 1/2/2017 1:27 PM, Claes Redestad wrote: Hi, during jigsaw bootstrap, package names - represented in external form, "java.lang" - are transformed into VM internal form, "java/lang", before calling into the VM. This, however, can effectively be moved into the VM, which removes the need to generate a lot of short-lived strings during bootstrap and heat up various String.replace-related methods. This has a noticeable impact on startup metrics. Webrev: http://cr.openjdk.java.net/~redestad/8171855/ Bug: https://bugs.openjdk.java.net/browse/JDK-8171855 JPRT -testset hotspot pass, and I've prepared to push this into jdk9/hs if there are no objections. Char-based replace on a UTF-8 source should be correct as long as the from and to chars are ASCII (0x00 through 0x7F). A more general method would be needed to safely deal with char values above 0x7F, so adding some asserts to that effect should be considered. Hi Claes, I have some concerns about this change in that it will break the precedence that currently the only internal form of names that the VM deals with are binary names as they appear in class files, where the periods (.) have been replaced by forward slashes (/). I think I would like to discuss this with the runtime team before you proceed. As far as the actual changes go, if we do proceed with this, there are places in modules.cpp that have been missed. After line #535, #665, #749 (the replace should be moved before the verify of the package name occurs), #820. Thanks, Lois Thanks! /Claes
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
Ok, I was sure to have seen java.base and not java.lang, past midnight here, time to crawle back to my bedroom :) Rémi - Mail original - > De: "Claes Redestad" > À: "Remi Forax" > Cc: "jigsaw-dev" , > hotspot-runtime-...@openjdk.java.net > Envoyé: Mardi 3 Janvier 2017 00:21:48 > Objet: Re: RFR: 8171855: Move package name transformations during module > bootstrap into VM > Hi Rémi, > > are you confusing package names with module names? > > From the linked specification: > > "Package names referenced from the Module attribute are stored in > CONSTANT_Package_info entries in the constant pool. Such entries wrap > CONSTANT_Utf8_info entries which represent package names encoded in > internal form." > > This still appears necessary since loading the module establishes the > symbol entries representing the referenced packages. > > Thanks! > > /Claes > > On 2017-01-03 00:06, Remi Forax wrote: >> Hi Claes, >> the whole replace dance should not be needed anymore with the latest >> revision of >> the spec, >> module names are not stored in internal form anymore. >> >> see http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.1 >> >> Rémi >> >> ----- Mail original - >>> De: "Claes Redestad" >>> À: "jigsaw-dev" , >>> hotspot-runtime-...@openjdk.java.net >>> Envoyé: Lundi 2 Janvier 2017 19:27:43 >>> Objet: RFR: 8171855: Move package name transformations during module >>> bootstrap >>> into VM >> >>> Hi, >>> >>> during jigsaw bootstrap, package names - represented in external form, >>> "java.lang" - are transformed into VM internal form, "java/lang", >>> before calling into the VM. >>> >>> This, however, can effectively be moved into the VM, which removes the >>> need to generate a lot of short-lived strings during bootstrap and heat >>> up various String.replace-related methods. This has a noticeable impact >>> on startup metrics. >>> >>> Webrev: http://cr.openjdk.java.net/~redestad/8171855/ >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8171855 >>> >>> JPRT -testset hotspot pass, and I've prepared to push this into jdk9/hs >>> if there are no objections. >>> >>> Char-based replace on a UTF-8 source should be correct as long as the >>> from and to chars are ASCII (0x00 through 0x7F). A more general method >>> would be needed to safely deal with char values above 0x7F, so adding >>> some asserts to that effect should be considered. >>> >>> Thanks! >>> > >> /Claes
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
Hi Rémi, are you confusing package names with module names? From the linked specification: "Package names referenced from the Module attribute are stored in CONSTANT_Package_info entries in the constant pool. Such entries wrap CONSTANT_Utf8_info entries which represent package names encoded in internal form." This still appears necessary since loading the module establishes the symbol entries representing the referenced packages. Thanks! /Claes On 2017-01-03 00:06, Remi Forax wrote: Hi Claes, the whole replace dance should not be needed anymore with the latest revision of the spec, module names are not stored in internal form anymore. see http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.1 Rémi - Mail original - De: "Claes Redestad" À: "jigsaw-dev" , hotspot-runtime-...@openjdk.java.net Envoyé: Lundi 2 Janvier 2017 19:27:43 Objet: RFR: 8171855: Move package name transformations during module bootstrap into VM Hi, during jigsaw bootstrap, package names - represented in external form, "java.lang" - are transformed into VM internal form, "java/lang", before calling into the VM. This, however, can effectively be moved into the VM, which removes the need to generate a lot of short-lived strings during bootstrap and heat up various String.replace-related methods. This has a noticeable impact on startup metrics. Webrev: http://cr.openjdk.java.net/~redestad/8171855/ Bug: https://bugs.openjdk.java.net/browse/JDK-8171855 JPRT -testset hotspot pass, and I've prepared to push this into jdk9/hs if there are no objections. Char-based replace on a UTF-8 source should be correct as long as the from and to chars are ASCII (0x00 through 0x7F). A more general method would be needed to safely deal with char values above 0x7F, so adding some asserts to that effect should be considered. Thanks! /Claes
Re: RFR: 8171855: Move package name transformations during module bootstrap into VM
Hi Claes, the whole replace dance should not be needed anymore with the latest revision of the spec, module names are not stored in internal form anymore. see http://cr.openjdk.java.net/~mr/jigsaw/spec/lang-vm.html#jigsaw-2.1 Rémi - Mail original - > De: "Claes Redestad" > À: "jigsaw-dev" , > hotspot-runtime-...@openjdk.java.net > Envoyé: Lundi 2 Janvier 2017 19:27:43 > Objet: RFR: 8171855: Move package name transformations during module > bootstrap into VM > Hi, > > during jigsaw bootstrap, package names - represented in external form, > "java.lang" - are transformed into VM internal form, "java/lang", > before calling into the VM. > > This, however, can effectively be moved into the VM, which removes the > need to generate a lot of short-lived strings during bootstrap and heat > up various String.replace-related methods. This has a noticeable impact > on startup metrics. > > Webrev: http://cr.openjdk.java.net/~redestad/8171855/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8171855 > > JPRT -testset hotspot pass, and I've prepared to push this into jdk9/hs > if there are no objections. > > Char-based replace on a UTF-8 source should be correct as long as the > from and to chars are ASCII (0x00 through 0x7F). A more general method > would be needed to safely deal with char values above 0x7F, so adding > some asserts to that effect should be considered. > > Thanks! > > /Claes
RFR: 8171855: Move package name transformations during module bootstrap into VM
Hi, during jigsaw bootstrap, package names - represented in external form, "java.lang" - are transformed into VM internal form, "java/lang", before calling into the VM. This, however, can effectively be moved into the VM, which removes the need to generate a lot of short-lived strings during bootstrap and heat up various String.replace-related methods. This has a noticeable impact on startup metrics. Webrev: http://cr.openjdk.java.net/~redestad/8171855/ Bug: https://bugs.openjdk.java.net/browse/JDK-8171855 JPRT -testset hotspot pass, and I've prepared to push this into jdk9/hs if there are no objections. Char-based replace on a UTF-8 source should be correct as long as the from and to chars are ASCII (0x00 through 0x7F). A more general method would be needed to safely deal with char values above 0x7F, so adding some asserts to that effect should be considered. Thanks! /Claes
