Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
Hi Alan, On 12/03/2013 8:05 PM, Alan Bateman wrote: On 12/03/2013 07:10, David Holmes wrote: : For the intro comment in profile-rtjar-includes.txt then it might be useful to beef up the comment to explain what happens when an API package does not match one of the rules, ie: does it go into compact1, only the full JRE, or none. Also make it explicit that the form DialogCallbackHandler*.class should not be used. I suggest this for the benefit of someone needing to tweak this in the future. I have updated the webrev with additional commentary. Thanks, that will be very useful to future maintainers. I have played with com.sun.tools.javac.sym.Profiles and so the changes to MakefileProfiles look okay to me but Jon should really be the reviewer for this. One thing about maxProfiles is that it should match Profile.values.length maybe maxProfiles should not be hardcoded to 4. Sorry but what is Profiles.values.length? Previously we inferred the number of profiles from the fact that we failed to find PROFILE_n for some value n. That can't work (easily) now hence the hard limit. I meant com.sun.tools.javac.jvm.Profile, it's an enum of the profiles so it means that the knowledge about 3 + full JRE is now in two places. I decided not to do this because it turns out that "4" is hardwired within the internal debugging code of this class anyway. I'll file a RFE to clean this up in conjunction with the additional testing you mention below. Thanks, David Another thing is whether to add a test or beef up an existing test (ProfileOptionTest.java in particular). What exactly is it that you would like to test for? I think the test should include a few cases to cover a few selected types in sub-packages to make sure they are in the right profile. -Alan.
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
The langtools changes look OK to me. -- Jon On 03/10/2013 06:24 PM, David Holmes wrote: I had overlooked the need to update the ct.sym creation tool to recognize the new syntax in the profile spec file. That process also uncovered a few bugs in the listing that needed correcting. The javadoc generation of compact profile information is not quite right, but that will be handled in a seperate bug. Updated webrevs at: http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/ --- http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/webrev.root/ No change from v1. Simply reverses the Nashorn change to $ processing. --- http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/webrev.langtools/ Updated the logic to look for FULL_JRE instead of PROFILE_4. And as a result of that needed to make the logic explicitly aware that there are 3 compact profiles plus the full JRE. --- http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/webrev.jdk/ Changes from v1: - I stopped trying to use a new wildcard syntax for included/excluded types. The only options are *.class else a full type name with $ replaced by $$. (The wildcard support would need more extensive changes to the ct.sym tool and for 4 classes it isn't worth it). - Types explicitly excluded from one profile must be explicitly included in a higher one (or the full JRE). (This happened to work by accident because we were dealing with compact3 and a full JRE). - The package sun/beans no longer exists - Fixed up include packages to capture new types ie jdk instead of jdk/internal so that we include the jdk.Supported annotation class - Removed additional redundant sub-packages Thanks for the follow up reviews. Erik's review stands as no further changes made to the actual build logic. David - On 8/03/2013 9:28 PM, David Holmes wrote: On 8/03/2013 7:19 PM, Alan Bateman wrote: On 08/03/2013 01:48, David Holmes wrote: Not sure which is best list for this given Alan will likely be the only reviewer anyway :) Webrevs under: http://cr.openjdk.java.net/~dholmes/8009428_8009429/ As further background to others, the reverting of the $ substitution became possible when Nashorn removed $ from its generated classes [1]. I've looked through the changes and they look really good to me. Putting $< in single quotes to avoid shell expansion is the solution that I could not find when adding the de-beaning recipes a few months ago. It's great to see PROFILE_4 renamed to FULL_JRE and is also great to remove the redundant sub-packages from the include lists. For profiles-rtjar-includes then I think it makes it much more readable and maintainable. Two minor inconsistencies: the sub-packages of javax.naming are listed and com/sun/jndi/ has a trailing slash. Fixed - well spotted! One probable side effect of removing the redundant sub-packages will be the javadoc. The one case to date where we didn't list sub-packages is java.time (because there are still changes going on there) and that the result is that the sub-packages of java.time don't show up in the profiles view. I created 8008367 a few weeks for that and I suspect that once these changes are pushed that the profiles view will be very incomplete. I don't think this should be a reason to hold off your changes as the profiles view isn't right now anyway. Hopefully Bhavesh will get to this soon. Now I'm a little concerned. I had not considered whether javac/javadoc considered these to be complete lists. They have to know how to combine the includes at a low-level with the excludes of a higher-level - and potentially vice-versa. I think Erik should review this too as it's important that the build changes in jdk8/tl will merge easily with changes going into jdk8/build. OK. Thanks, David -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/fe5211fc3114
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
On 12/03/2013 07:10, David Holmes wrote: : For the intro comment in profile-rtjar-includes.txt then it might be useful to beef up the comment to explain what happens when an API package does not match one of the rules, ie: does it go into compact1, only the full JRE, or none. Also make it explicit that the form DialogCallbackHandler*.class should not be used. I suggest this for the benefit of someone needing to tweak this in the future. I have updated the webrev with additional commentary. Thanks, that will be very useful to future maintainers. I have played with com.sun.tools.javac.sym.Profiles and so the changes to MakefileProfiles look okay to me but Jon should really be the reviewer for this. One thing about maxProfiles is that it should match Profile.values.length maybe maxProfiles should not be hardcoded to 4. Sorry but what is Profiles.values.length? Previously we inferred the number of profiles from the fact that we failed to find PROFILE_n for some value n. That can't work (easily) now hence the hard limit. I meant com.sun.tools.javac.jvm.Profile, it's an enum of the profiles so it means that the knowledge about 3 + full JRE is now in two places. Another thing is whether to add a test or beef up an existing test (ProfileOptionTest.java in particular). What exactly is it that you would like to test for? I think the test should include a few cases to cover a few selected types in sub-packages to make sure they are in the right profile. -Alan.
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
Hi Alan, On 11/03/2013 6:54 PM, Alan Bateman wrote: On 11/03/2013 01:24, David Holmes wrote: I had overlooked the need to update the ct.sym creation tool to recognize the new syntax in the profile spec file. That process also uncovered a few bugs in the listing that needed correcting. The javadoc generation of compact profile information is not quite right, but that will be handled in a seperate bug. Updated webrevs at: http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/ The changes to the top-level and jdk repo look good to me. As per my original comments, I'm happy to see profile-rtjar-includes.txt changed to be much easier to maintain. The assumption that jdk.** is in compact1 is okay for now but I'm sure that will need to be changed when that namespace is used more. Yes we will change this as we need to, once other packages come into play. For the intro comment in profile-rtjar-includes.txt then it might be useful to beef up the comment to explain what happens when an API package does not match one of the rules, ie: does it go into compact1, only the full JRE, or none. Also make it explicit that the form DialogCallbackHandler*.class should not be used. I suggest this for the benefit of someone needing to tweak this in the future. I have updated the webrev with additional commentary. I have played with com.sun.tools.javac.sym.Profiles and so the changes to MakefileProfiles look okay to me but Jon should really be the reviewer for this. One thing about maxProfiles is that it should match Profile.values.length maybe maxProfiles should not be hardcoded to 4. Sorry but what is Profiles.values.length? Previously we inferred the number of profiles from the fact that we failed to find PROFILE_n for some value n. That can't work (easily) now hence the hard limit. Another thing is whether to add a test or beef up an existing test (ProfileOptionTest.java in particular). What exactly is it that you would like to test for? Thanks, David -Alan.
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
On 11/03/2013 01:24, David Holmes wrote: I had overlooked the need to update the ct.sym creation tool to recognize the new syntax in the profile spec file. That process also uncovered a few bugs in the listing that needed correcting. The javadoc generation of compact profile information is not quite right, but that will be handled in a seperate bug. Updated webrevs at: http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/ The changes to the top-level and jdk repo look good to me. As per my original comments, I'm happy to see profile-rtjar-includes.txt changed to be much easier to maintain. The assumption that jdk.** is in compact1 is okay for now but I'm sure that will need to be changed when that namespace is used more. For the intro comment in profile-rtjar-includes.txt then it might be useful to beef up the comment to explain what happens when an API package does not match one of the rules, ie: does it go into compact1, only the full JRE, or none. Also make it explicit that the form DialogCallbackHandler*.class should not be used. I suggest this for the benefit of someone needing to tweak this in the future. I have played with com.sun.tools.javac.sym.Profiles and so the changes to MakefileProfiles look okay to me but Jon should really be the reviewer for this. One thing about maxProfiles is that it should match Profile.values.length maybe maxProfiles should not be hardcoded to 4. Another thing is whether to add a test or beef up an existing test (ProfileOptionTest.java in particular). -Alan.
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
I had overlooked the need to update the ct.sym creation tool to recognize the new syntax in the profile spec file. That process also uncovered a few bugs in the listing that needed correcting. The javadoc generation of compact profile information is not quite right, but that will be handled in a seperate bug. Updated webrevs at: http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/ --- http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/webrev.root/ No change from v1. Simply reverses the Nashorn change to $ processing. --- http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/webrev.langtools/ Updated the logic to look for FULL_JRE instead of PROFILE_4. And as a result of that needed to make the logic explicitly aware that there are 3 compact profiles plus the full JRE. --- http://cr.openjdk.java.net/~dholmes/8009428_8009429.v2/webrev.jdk/ Changes from v1: - I stopped trying to use a new wildcard syntax for included/excluded types. The only options are *.class else a full type name with $ replaced by $$. (The wildcard support would need more extensive changes to the ct.sym tool and for 4 classes it isn't worth it). - Types explicitly excluded from one profile must be explicitly included in a higher one (or the full JRE). (This happened to work by accident because we were dealing with compact3 and a full JRE). - The package sun/beans no longer exists - Fixed up include packages to capture new types ie jdk instead of jdk/internal so that we include the jdk.Supported annotation class - Removed additional redundant sub-packages Thanks for the follow up reviews. Erik's review stands as no further changes made to the actual build logic. David - On 8/03/2013 9:28 PM, David Holmes wrote: On 8/03/2013 7:19 PM, Alan Bateman wrote: On 08/03/2013 01:48, David Holmes wrote: Not sure which is best list for this given Alan will likely be the only reviewer anyway :) Webrevs under: http://cr.openjdk.java.net/~dholmes/8009428_8009429/ As further background to others, the reverting of the $ substitution became possible when Nashorn removed $ from its generated classes [1]. I've looked through the changes and they look really good to me. Putting $< in single quotes to avoid shell expansion is the solution that I could not find when adding the de-beaning recipes a few months ago. It's great to see PROFILE_4 renamed to FULL_JRE and is also great to remove the redundant sub-packages from the include lists. For profiles-rtjar-includes then I think it makes it much more readable and maintainable. Two minor inconsistencies: the sub-packages of javax.naming are listed and com/sun/jndi/ has a trailing slash. Fixed - well spotted! One probable side effect of removing the redundant sub-packages will be the javadoc. The one case to date where we didn't list sub-packages is java.time (because there are still changes going on there) and that the result is that the sub-packages of java.time don't show up in the profiles view. I created 8008367 a few weeks for that and I suspect that once these changes are pushed that the profiles view will be very incomplete. I don't think this should be a reason to hold off your changes as the profiles view isn't right now anyway. Hopefully Bhavesh will get to this soon. Now I'm a little concerned. I had not considered whether javac/javadoc considered these to be complete lists. They have to know how to combine the includes at a low-level with the excludes of a higher-level - and potentially vice-versa. I think Erik should review this too as it's important that the build changes in jdk8/tl will merge easily with changes going into jdk8/build. OK. Thanks, David -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/fe5211fc3114
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
On 08/03/2013 11:28, David Holmes wrote: Now I'm a little concerned. I had not considered whether javac/javadoc considered these to be complete lists. They have to know how to combine the includes at a low-level with the excludes of a higher-level - and potentially vice-versa. I think javac should be okay, and you can easily test this by trying to compile with "javac -profile compact ..." on a test that references types in one of these sub-packages. There are tests in the langtools repo for this too and it would be good to check ProfileOptionTest to see if need more sub-tests to cover a few sample types from these sub-packages. So I think the issue will be just the docs build and you can quickly check that too. To date we've only seen it with java.time because that was the one case where only the top-most API package was listed. I'm pretty sure the issue is in the standard doclet, in which case I think pushing your changes are okay. The only complication is that there is another issue with the generated docs and that is default view is the Profiles view so it is very confusing until you hit the "All Packages" link. -Alan
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
Thanks Erik! David On 8/03/2013 9:25 PM, Erik Joelsson wrote: On 2013-03-08 10:19, Alan Bateman wrote: On 08/03/2013 01:48, David Holmes wrote: Not sure which is best list for this given Alan will likely be the only reviewer anyway :) Webrevs under: http://cr.openjdk.java.net/~dholmes/8009428_8009429/ As further background to others, the reverting of the $ substitution became possible when Nashorn removed $ from its generated classes [1]. I've looked through the changes and they look really good to me. Putting $< in single quotes to avoid shell expansion is the solution that I could not find when adding the de-beaning recipes a few months ago. It's great to see PROFILE_4 renamed to FULL_JRE and is also great to remove the redundant sub-packages from the include lists. For profiles-rtjar-includes then I think it makes it much more readable and maintainable. Two minor inconsistencies: the sub-packages of javax.naming are listed and com/sun/jndi/ has a trailing slash. One probable side effect of removing the redundant sub-packages will be the javadoc. The one case to date where we didn't list sub-packages is java.time (because there are still changes going on there) and that the result is that the sub-packages of java.time don't show up in the profiles view. I created 8008367 a few weeks for that and I suspect that once these changes are pushed that the profiles view will be very incomplete. I don't think this should be a reason to hold off your changes as the profiles view isn't right now anyway. Hopefully Bhavesh will get to this soon. I think Erik should review this too as it's important that the build changes in jdk8/tl will merge easily with changes going into jdk8/build. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/fe5211fc3114 Looks good to me and thanks again for cleaning up that $ business. I do not think this will conflict with anything coming from jdk8/build. /Erik
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
On 8/03/2013 7:19 PM, Alan Bateman wrote: On 08/03/2013 01:48, David Holmes wrote: Not sure which is best list for this given Alan will likely be the only reviewer anyway :) Webrevs under: http://cr.openjdk.java.net/~dholmes/8009428_8009429/ As further background to others, the reverting of the $ substitution became possible when Nashorn removed $ from its generated classes [1]. I've looked through the changes and they look really good to me. Putting $< in single quotes to avoid shell expansion is the solution that I could not find when adding the de-beaning recipes a few months ago. It's great to see PROFILE_4 renamed to FULL_JRE and is also great to remove the redundant sub-packages from the include lists. For profiles-rtjar-includes then I think it makes it much more readable and maintainable. Two minor inconsistencies: the sub-packages of javax.naming are listed and com/sun/jndi/ has a trailing slash. Fixed - well spotted! One probable side effect of removing the redundant sub-packages will be the javadoc. The one case to date where we didn't list sub-packages is java.time (because there are still changes going on there) and that the result is that the sub-packages of java.time don't show up in the profiles view. I created 8008367 a few weeks for that and I suspect that once these changes are pushed that the profiles view will be very incomplete. I don't think this should be a reason to hold off your changes as the profiles view isn't right now anyway. Hopefully Bhavesh will get to this soon. Now I'm a little concerned. I had not considered whether javac/javadoc considered these to be complete lists. They have to know how to combine the includes at a low-level with the excludes of a higher-level - and potentially vice-versa. I think Erik should review this too as it's important that the build changes in jdk8/tl will merge easily with changes going into jdk8/build. OK. Thanks, David -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/fe5211fc3114
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
On 2013-03-08 10:19, Alan Bateman wrote: On 08/03/2013 01:48, David Holmes wrote: Not sure which is best list for this given Alan will likely be the only reviewer anyway :) Webrevs under: http://cr.openjdk.java.net/~dholmes/8009428_8009429/ As further background to others, the reverting of the $ substitution became possible when Nashorn removed $ from its generated classes [1]. I've looked through the changes and they look really good to me. Putting $< in single quotes to avoid shell expansion is the solution that I could not find when adding the de-beaning recipes a few months ago. It's great to see PROFILE_4 renamed to FULL_JRE and is also great to remove the redundant sub-packages from the include lists. For profiles-rtjar-includes then I think it makes it much more readable and maintainable. Two minor inconsistencies: the sub-packages of javax.naming are listed and com/sun/jndi/ has a trailing slash. One probable side effect of removing the redundant sub-packages will be the javadoc. The one case to date where we didn't list sub-packages is java.time (because there are still changes going on there) and that the result is that the sub-packages of java.time don't show up in the profiles view. I created 8008367 a few weeks for that and I suspect that once these changes are pushed that the profiles view will be very incomplete. I don't think this should be a reason to hold off your changes as the profiles view isn't right now anyway. Hopefully Bhavesh will get to this soon. I think Erik should review this too as it's important that the build changes in jdk8/tl will merge easily with changes going into jdk8/build. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/fe5211fc3114 Looks good to me and thanks again for cleaning up that $ business. I do not think this will conflict with anything coming from jdk8/build. /Erik
Re: RFR: 8009428 and 8009429 - Profile related fixes and clean ups
On 08/03/2013 01:48, David Holmes wrote: Not sure which is best list for this given Alan will likely be the only reviewer anyway :) Webrevs under: http://cr.openjdk.java.net/~dholmes/8009428_8009429/ As further background to others, the reverting of the $ substitution became possible when Nashorn removed $ from its generated classes [1]. I've looked through the changes and they look really good to me. Putting $< in single quotes to avoid shell expansion is the solution that I could not find when adding the de-beaning recipes a few months ago. It's great to see PROFILE_4 renamed to FULL_JRE and is also great to remove the redundant sub-packages from the include lists. For profiles-rtjar-includes then I think it makes it much more readable and maintainable. Two minor inconsistencies: the sub-packages of javax.naming are listed and com/sun/jndi/ has a trailing slash. One probable side effect of removing the redundant sub-packages will be the javadoc. The one case to date where we didn't list sub-packages is java.time (because there are still changes going on there) and that the result is that the sub-packages of java.time don't show up in the profiles view. I created 8008367 a few weeks for that and I suspect that once these changes are pushed that the profiles view will be very incomplete. I don't think this should be a reason to hold off your changes as the profiles view isn't right now anyway. Hopefully Bhavesh will get to this soon. I think Erik should review this too as it's important that the build changes in jdk8/tl will merge easily with changes going into jdk8/build. -Alan. [1] http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/fe5211fc3114
