Hi Goetz,
this looks good to me. Thanks for making these tests more robust.
Best regards
Christoph
> -Original Message-
> From: serviceability-dev [mailto:serviceability-dev-
> [email protected]] On Behalf Of Lindenmaier, Goetz
> Sent: Montag, 11. Juni 2018 15:15
> To: serviceabil
Hi Serguei,
thank you for your review!
You are right, a regression test makes sense here. I wrote one. See
updated webrev (only added the test, nothing else did change):
http://cr.openjdk.java.net/~stuefe/webrevs/8203343-VM.metaspace-show-reflection-invocation-targets/webrev.01/webrev/
I am re-
PING: Could you review it?
I want to merge this change before JDK 11 RDP 1.
webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.00/
JBS:
https://bugs.openjdk.java.net/browse/JDK-8204531
Yasumasa
On 2018/06/08 15:38, Yasumasa Suenaga wrote:
Hi David,
2018-0
Hi Yasumasa,
your patch will change behavior of jsnap if file.encoding is set to
something different than US-ASCII.
Old version uses, hard wired, US-ASCII.
You now use jvm/hotspot/utilities/CStringUtilities.java, which uses
private static String encoding = System.getProperty("file.encoding",
"U
Hi Thomas,
Thank you for your comment.
CStringUtilities.java says the String which is constructed from byte array
should use Charset.defaultCharset(). [1]
I'm not convinced how should we fix - US-ASCII or default charset.
IMHO we should use default charset. However I concern I need to get CSR
Disclaimer: I am Reviewer but not versed in this corner of the jdk,
nor have knowledge of the history, so take what I suggest with a grain
of salt.
That said, I'd probably just add a variant to
CStringUtilities::getString which takes encoding as an argument, and
then pass "US-ASCII" to remain comp
Thanks Thomas,
That said, I'd probably just add a variant to
CStringUtilities::getString which takes encoding as an argument, and
then pass "US-ASCII" to remain compatible with the current version of
jsnap.
I agree with you. My opinion is one of the ideals.
Anyway, I uploaded new webrev which
Hi all,
while working on jcmd I saw some minor cleanup possibilities.
Webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8204958
Most of them are const correctness and using initializer lists instead
of java-s
(jdk-submit tests came back clean)
On Wed, Jun 13, 2018 at 3:41 PM, Thomas Stüfe wrote:
> Hi all,
>
> while working on jcmd I saw some minor cleanup possibilities.
>
> Webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/
> Bug: https://bugs.openjdk.java.ne
Hi Yasumasa,
looks good to me.
small nit:
import java.nio.charset.* -> import java.nio.charset.Charset;
We loose the assert for type now from byteArrayValue(). Can you re-add
it to the location where you call CStringUtils? We do not seem to need
byteArrayValue(), can this be removed?
Best Regar
Hi Thomas,
I looks good to me.
A couple of minor comments.
http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/src/hotspot/share/services/diagnosticArgument.hpp.udiff.html
- void value_as_str(char *buf, size_t len) { r
On 6/13/18 4:11 PM,
[email protected] wrote:
Hi Thomas,
I looks good to me.
A couple of minor comments.
http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/src/hotspot/s
On 6/13/18 16:44, Chris Plummer wrote:
On 6/13/18 4:11 PM, [email protected] wrote:
Hi Thomas,
I looks good to me.
A couple of minor comments.
http://cr.openjdk.java.net/~
Hi Thomas,
Thank you for your comment.
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.02/
2018-06-14 3:34 GMT+09:00 Thomas Stüfe :
> Hi Yasumasa,
>
> looks good to me.
>
> small nit:
> import java.nio.charset.* -> import java.nio.charset.Charset;
Fixed.
> We
On 6/13/18 6:11 PM,
[email protected] wrote:
On 6/13/18 16:44, Chris Plummer
wrote:
On 6/13/18 4:11 PM, [email protected] wrote:
Hi Thomas,
I looks g
Hi Yasumasa,
This seems mostly okay - though like Thomas this is not an area I'm that
familiar with. But it seems this addresses the problem without impacting
other things.
This assert is redundant:
366 if (Assert.ASSERTS_ENABLED) {
367 Assert.that(vecto
On 6/13/18 21:19, Chris Plummer wrote:
On 6/13/18 6:11 PM, [email protected] wrote:
On 6/13/18 16:44, Chris Plummer
wrote:
On 6/13/18 4:11 PM, [email protected]
wrote:
On Thu, Jun 14, 2018 at 6:19 AM, Chris Plummer wrote:
> On 6/13/18 6:11 PM, [email protected] wrote:
>
> On 6/13/18 16:44, Chris Plummer wrote:
>
> On 6/13/18 4:11 PM, [email protected] wrote:
>
> Hi Thomas,
>
> I looks good to me.
>
> A couple of minor comments.
>
> http://cr.op
Hi David,
Thank you for your comment.
I removed the assertion from PerfDataEntry.java .
Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.03/
Thanks,
Yasuamsa
2018-06-14 13:41 GMT+09:00 David Holmes :
> Hi Yasumasa,
>
> This seems mostly okay - though like T
On 6/13/18 9:53 PM,
[email protected] wrote:
On 6/13/18 21:19, Chris Plummer
wrote:
On 6/13/18 6:11 PM, [email protected] wrote:
On 6/13/18 16:44, Chris Plummer
w
Hi Serguei,
On Thu, Jun 14, 2018 at 1:11 AM, [email protected]
wrote:
> Hi Thomas,
>
> I looks good to me.
>
> A couple of minor comments.
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8204958-jcmd-cleanups/webrev.00/webrev/src/hotspot/share/services/diagnosticArgument.hpp.udiff.html
>
Hi Thomas,
Just a nit in the test:
54 // Do some reflection, enough times to hit the
sun.reflect.inflationThreshold and force
55 // generation of reflection accessor classes.
...
59 for (int i = 0; i < 100; i ++) {
The default threshold is only 15. The test is
Hi Chris,
On Thu, Jun 14, 2018 at 7:01 AM, Chris Plummer wrote:
> On 6/13/18 9:53 PM, [email protected] wrote:
>
> On 6/13/18 21:19, Chris Plummer wrote:
>
> On 6/13/18 6:11 PM, [email protected] wrote:
>
> On 6/13/18 16:44, Chris Plummer wrote:
>
> On 6/13/18 4:11 PM, serguei.s
On Thu, Jun 14, 2018 at 7:11 AM, David Holmes wrote:
> Hi Thomas,
>
> Just a nit in the test:
>
> 54 // Do some reflection, enough times to hit the
> sun.reflect.inflationThreshold and force
> 55 // generation of reflection accessor classes.
>...
> 59 for (int i =
Seems fine.
Thanks,
David
On 14/06/2018 3:01 PM, Yasumasa Suenaga wrote:
Hi David,
Thank you for your comment.
I removed the assertion from PerfDataEntry.java .
Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8204531/webrev.03/
Thanks,
Yasuamsa
2018-06-14 13:41 GMT+
25 matches
Mail list logo
