Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Thank you very much, Chris, for taking a look. Yes, I have the arch specific check since VM_Version::CPU_SHA is cpu specific. I chose one platform independent long constant -- "markOopDesc::hash_mask_in_place". This is the only platform independent long Constant with which the truncation issue can be observed. I added a platform dependent long constant also to get some extra testing done. I will add the comments for the new code. Thank you, Jini. On 5/12/2018 6:30 AM, Chris Plummer wrote: Hi Jini, Why do you have the following: 98 if (arch.equals("amd64") || arch.equals("i386") || arch.equals("x86")) { Is it because VM_Version::CPU_SHA is only expected on these platforms? If so, is there a reason you didn't choose a long constant that exists on all platforms? And just one minor nit. I think the new code warrants some comments indicating what it is trying to test for. thanks, Chris On 5/11/18 5:11 AM, Jini George wrote: Thank you, David. Could I get another pair of eyes to take a look at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html Thanks, Jini. On 5/11/2018 11:58 AM, David Holmes wrote: Forgot to add - no need to see an updated webrev. Thanks, David On 11/05/2018 4:22 PM, David Holmes wrote: Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Thank you very much, Serguei. Will add it. - Jini. On 5/12/2018 6:29 AM, serguei.spit...@oracle.com wrote: Hi Jini, It looks good to me. A minor comment on the ClhsdbLongConstant.java.frames.html: Could you add a comment with an example of the expected longConstantOutput? I do not need another webrev if you add it. Thanks, Serguei On 5/11/18 05:11, Jini George wrote: Thank you, David. Could I get another pair of eyes to take a look at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html Thanks, Jini. On 5/11/2018 11:58 AM, David Holmes wrote: Forgot to add - no need to see an updated webrev. Thanks, David On 11/05/2018 4:22 PM, David Holmes wrote: Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini, Why do you have the following: 98 if (arch.equals("amd64") || arch.equals("i386") || arch.equals("x86")) { Is it because VM_Version::CPU_SHA is only expected on these platforms? If so, is there a reason you didn't choose a long constant that exists on all platforms? And just one minor nit. I think the new code warrants some comments indicating what it is trying to test for. thanks, Chris On 5/11/18 5:11 AM, Jini George wrote: Thank you, David. Could I get another pair of eyes to take a look at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html Thanks, Jini. On 5/11/2018 11:58 AM, David Holmes wrote: Forgot to add - no need to see an updated webrev. Thanks, David On 11/05/2018 4:22 PM, David Holmes wrote: Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini, It looks good to me. A minor comment on the ClhsdbLongConstant.java.frames.html: Could you add a comment with an example of the expected longConstantOutput? I do not need another webrev if you add it. Thanks, Serguei On 5/11/18 05:11, Jini George wrote: Thank you, David. Could I get another pair of eyes to take a look at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html Thanks, Jini. On 5/11/2018 11:58 AM, David Holmes wrote: Forgot to add - no need to see an updated webrev. Thanks, David On 11/05/2018 4:22 PM, David Holmes wrote: Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Thank you, David. Could I get another pair of eyes to take a look at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.02/index.html Thanks, Jini. On 5/11/2018 11:58 AM, David Holmes wrote: Forgot to add - no need to see an updated webrev. Thanks, David On 11/05/2018 4:22 PM, David Holmes wrote: Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Forgot to add - no need to see an updated webrev. Thanks, David On 11/05/2018 4:22 PM, David Holmes wrote: Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini, On 11/05/2018 5:34 AM, Jini George wrote: Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Looks fine. A couple of minor comments on the test: You don't need: 79 LingeredApp.stopApp(theApp); as it is done in the finally block. Can you add comments in checkForTruncation stating where the expected values were obtained from. Thanks, David Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi David, Thank you very much for the review and for the clarifications offline! I have addressed your comments and have a new webrev at: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.01/index.html Thanks, Jini. On 5/4/2018 12:23 PM, David Holmes wrote: Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.
Re: RFR: (S): JDK-8195613: [SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int
Hi Jini, On 4/05/2018 2:17 AM, Jini George wrote: Hello! Requesting reviews for: https://bugs.openjdk.java.net/browse/JDK-8195613 ([SA] HotSpotTypeDataBase.readVMLongConstants truncates values to int) Webrev: http://cr.openjdk.java.net/~jgeorge/8195613/webrev.00/ Actual fix seems fine. I have a few comments on the test ... - Why are you only testing on 64-bit? The truncation would happen on 32-bit as well. - CheckForTruncation seems rather complicated, not sure why we need to look for two different things with one being amd64 specific?? - We've had discussions in the past about splitting strings on \n or \r\n depending on whether it's Windows or not. Better to use String.split("\R") regex function. - Exception message strings should not contain newline characters. Also you can simplify them: Reading XXX: got value NNN but expected MMM And you don't need to define a local variable "String message" you can just: throw new Exception("Reading XXX: got value " + value + " but expected " + expected); Thanks, David - Testing: The SA tests passed on Mach5. Thanks, Jini.