Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Dmitry Samersoff]

Paul,

Thank you!

-Dmitry


On 31.10.2017 20:32, Paul Sandoz wrote:
> 
>> On 31 Oct 2017, at 05:58, Dmitry Samersoff <dmitry.samers...@bell-sw.com> 
>> wrote:
>>
>> Paul and Frederic,
>>
>> Thank you.
>>
>> One more question. Do we need to call verify_oop below?
>>
>> 509   { // Check for the null sentinel.
>> ...
>> 517  xorptr(result, result);  // NULL object reference
>> ...
>>
>> 521   if (VerifyOops) {
>> 522  verify_oop(result);
>> 523   }
>>
> 
> I believe it’s harmless.
> 
> When the flag is on it eventually results in a call to the stub generated by 
> generate_verify_oop:
> 
> http://hg.openjdk.java.net/jdk10/hs/file/tip/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp#l1023
> 
> // make sure object is 'reasonable'
> __ testptr(rax, rax);
> __ jcc(Assembler::zero, exit); // if obj is NULL it is OK
> 
> If the oop is null the verification will exit safely.
> 
> Paul.
> 
>> -Dmitry
>>
>>
>> On 31.10.2017 00:56, Frederic Parain wrote:
>>> I’m seeing no issue with rcx being aliased in this code.
>>>
>>> Fred
>>>
>>>> On Oct 30, 2017, at 15:44, Paul Sandoz <paul.san...@oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Thanks for reviewing.
>>>>
>>>>> On 30 Oct 2017, at 11:05, Dmitry Samersoff <dmitry.samers...@bell-sw.com> 
>>>>> wrote:
>>>>>
>>>>> Paul,
>>>>>
>>>>> templateTable_x86.cpp:
>>>>>
>>>>> 564   const Register flags = rcx;
>>>>> 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>>>>>
>>>>> Should we use another register for rarg under NOT_LP64 ?
>>>>>
>>>>
>>>> I think it should be ok, it i ain’t an expert here on the interpreter and 
>>>> the calling conventions, so please correct me.
>>>>
>>>> Some more context:
>>>>
>>>> +  const Register flags = rcx;
>>>> +  const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>>>> +  __ movl(rarg, (int)bytecode());
>>>>
>>>> The current bytecode code is loaded into “rarg”
>>>>
>>>> +  call_VM(obj, CAST_FROM_FN_PTR(address, 
>>>> InterpreterRuntime::resolve_ldc), rarg);
>>>>
>>>> Then “rarg" is the argument to the call to 
>>>> InterpreterRuntime::resolve_ldc, after which it is no longer referred to.
>>>>
>>>> +#ifndef _LP64
>>>> +  // borrow rdi from locals
>>>> +  __ get_thread(rdi);
>>>> +  __ get_vm_result_2(flags, rdi);
>>>> +  __ restore_locals();
>>>> +#else
>>>> +  __ get_vm_result_2(flags, r15_thread);
>>>> +#endif
>>>>
>>>> The result from the call is then loaded into flags.
>>>>
>>>> So i don’t think it matters in this case if rcx is aliased.
>>>>
>>>> Paul.
>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 10/26/2017 08:03 PM, Paul Sandoz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the following patch for minimal dynamic constant support:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>>>>>>  
>>>>>> <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8186046 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8186046>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8186209 
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8186209>
>>>>>>
>>>>>> This patch is based on the JDK 10 unified HotSpot repository. Testing so 
>>>>>> far looks good.
>>>>>>
>>>>>> By minimal i mean just the support in the runtime for a dynamic constant 
>>>>>> pool entry to be referenced by a LDC instruction or a bootstrap method 
>>>>>> argument. Much of the work leverages the foundations built by invoke 
>>>>>> dynamic but is arguably simpler since resolution is less complex.
>>>>>>
>>>>>> A small set of bootstrap methods will be proposed as a follow on issue 
>&g

Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Dmitry Samersoff]

Paul,

templateTable_x86.cpp:

 564   const Register flags = rcx;
 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);

Should we use another register for rarg under NOT_LP64 ?

-Dmitry


On 10/26/2017 08:03 PM, Paul Sandoz wrote:
> Hi,
> 
> Please review the following patch for minimal dynamic constant support:
> 
>   
> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>  
> 
> 
>   https://bugs.openjdk.java.net/browse/JDK-8186046 
> 
>   https://bugs.openjdk.java.net/browse/JDK-8186209 
> 
> 
> This patch is based on the JDK 10 unified HotSpot repository. Testing so far 
> looks good.
> 
> By minimal i mean just the support in the runtime for a dynamic constant pool 
> entry to be referenced by a LDC instruction or a bootstrap method argument. 
> Much of the work leverages the foundations built by invoke dynamic but is 
> arguably simpler since resolution is less complex.
> 
> A small set of bootstrap methods will be proposed as a follow on issue for 10 
> (these are currently being refined in the amber repository).
> 
> Bootstrap method invocation has not changed (and the rules are the same for 
> dynamic constants and indy). It is planned to enhance this in a further major 
> release to support lazy resolution of bootstrap method arguments.
> 
> The CSR for the VM specification is here:
> 
>   https://bugs.openjdk.java.net/browse/JDK-8189199 
> 
> 
> the j.l.invoke package documentation was also updated but please consider the 
> VM specification as the definitive "source of truth" (we may clean up this 
> area further later on so it becomes more informative, and that may also apply 
> to duplicative text on MethodHandles/VarHandles).
> 
> Any AoT-related work will be deferred to a future release.
> 
> —
> 
> This patch only supports x64 platforms. There is a small set of changes 
> specific to x64 (specifically to support null and primitives constants, as 
> prior to this patch null was used as a sentinel for resolution and certain 
> primitives types would never have been encountered, such as say byte).
> 
> We will need to follow up with the SPARC platform and it is hoped/anticipated 
> that OpenJDK members responsible for other platforms (namely ARM and PPC) 
> will separately provide patches.
> 
> —
> 
> Many of tests rely on an experimental byte code API that supports the 
> generation of byte code with dynamic constants.
> 
> One test uses class file bytes produced from a modified version of asmtools.  
> The modifications have now been pushed but a new version of asmtools need to 
> be rolled into jtreg before the test can operate directly on asmtools 
> information rather than embedding class file bytes directly in the test.
> 
> —
> 
> Paul.
> 

Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Dmitry Samersoff]

Paul and Frederic,

Thank you.

One more question. Do we need to call verify_oop below?

509   { // Check for the null sentinel.
...
517  xorptr(result, result);  // NULL object reference
...

521   if (VerifyOops) {
522  verify_oop(result);
523   }

-Dmitry


On 31.10.2017 00:56, Frederic Parain wrote:
> I’m seeing no issue with rcx being aliased in this code.
> 
> Fred
> 
>> On Oct 30, 2017, at 15:44, Paul Sandoz <paul.san...@oracle.com> wrote:
>>
>> Hi,
>>
>> Thanks for reviewing.
>>
>>> On 30 Oct 2017, at 11:05, Dmitry Samersoff <dmitry.samers...@bell-sw.com> 
>>> wrote:
>>>
>>> Paul,
>>>
>>> templateTable_x86.cpp:
>>>
>>> 564   const Register flags = rcx;
>>> 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>>>
>>> Should we use another register for rarg under NOT_LP64 ?
>>>
>>
>> I think it should be ok, it i ain’t an expert here on the interpreter and 
>> the calling conventions, so please correct me.
>>
>> Some more context:
>>
>> +  const Register flags = rcx;
>> +  const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>> +  __ movl(rarg, (int)bytecode());
>>
>> The current bytecode code is loaded into “rarg”
>>
>> +  call_VM(obj, CAST_FROM_FN_PTR(address, InterpreterRuntime::resolve_ldc), 
>> rarg);
>>
>> Then “rarg" is the argument to the call to InterpreterRuntime::resolve_ldc, 
>> after which it is no longer referred to.
>>
>> +#ifndef _LP64
>> +  // borrow rdi from locals
>> +  __ get_thread(rdi);
>> +  __ get_vm_result_2(flags, rdi);
>> +  __ restore_locals();
>> +#else
>> +  __ get_vm_result_2(flags, r15_thread);
>> +#endif
>>
>> The result from the call is then loaded into flags.
>>
>> So i don’t think it matters in this case if rcx is aliased.
>>
>> Paul.
>>
>>> -Dmitry
>>>
>>>
>>> On 10/26/2017 08:03 PM, Paul Sandoz wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch for minimal dynamic constant support:
>>>>
>>>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>>>>  
>>>> <http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/>
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8186046 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8186046>
>>>> https://bugs.openjdk.java.net/browse/JDK-8186209 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8186209>
>>>>
>>>> This patch is based on the JDK 10 unified HotSpot repository. Testing so 
>>>> far looks good.
>>>>
>>>> By minimal i mean just the support in the runtime for a dynamic constant 
>>>> pool entry to be referenced by a LDC instruction or a bootstrap method 
>>>> argument. Much of the work leverages the foundations built by invoke 
>>>> dynamic but is arguably simpler since resolution is less complex.
>>>>
>>>> A small set of bootstrap methods will be proposed as a follow on issue for 
>>>> 10 (these are currently being refined in the amber repository).
>>>>
>>>> Bootstrap method invocation has not changed (and the rules are the same 
>>>> for dynamic constants and indy). It is planned to enhance this in a 
>>>> further major release to support lazy resolution of bootstrap method 
>>>> arguments.
>>>>
>>>> The CSR for the VM specification is here:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8189199 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8189199>
>>>>
>>>> the j.l.invoke package documentation was also updated but please consider 
>>>> the VM specification as the definitive "source of truth" (we may clean up 
>>>> this area further later on so it becomes more informative, and that may 
>>>> also apply to duplicative text on MethodHandles/VarHandles).
>>>>
>>>> Any AoT-related work will be deferred to a future release.
>>>>
>>>> —
>>>>
>>>> This patch only supports x64 platforms. There is a small set of changes 
>>>> specific to x64 (specifically to support null and primitives constants, as 
>>>> prior to this patch null was used as a sentinel for resolution and certain 
>>>> primitives types would never have been encountered, such as say byte).
>>>>
>>>> We will need to follow up with the SPARC platform and it is 
>>>> hoped/anticipated that OpenJDK members responsible for other platforms 
>>>> (namely ARM and PPC) will separately provide patches.
>>>>
>>>> —
>>>>
>>>> Many of tests rely on an experimental byte code API that supports the 
>>>> generation of byte code with dynamic constants.
>>>>
>>>> One test uses class file bytes produced from a modified version of 
>>>> asmtools.  The modifications have now been pushed but a new version of 
>>>> asmtools need to be rolled into jtreg before the test can operate directly 
>>>> on asmtools information rather than embedding class file bytes directly in 
>>>> the test.
>>>>
>>>> —
>>>>
>>>> Paul.
>>>>
>>>
>>
> 

Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent

[Dmitry Samersoff]

Matthias,

Looks good to me.

-Dmitry

On 2017-05-16 13:21, Baesken, Matthias wrote:
>  Sure, I forward it to  serviceability-dev .
> 
> -Original Message-
> From: Alan Bateman [mailto:alan.bate...@oracle.com] 
> Sent: Dienstag, 16. Mai 2017 11:51
> To: Baesken, Matthias <matthias.baes...@sap.com>; 
> core-libs-dev@openjdk.java.net
> Cc: Simonis, Volker <volker.simo...@sap.com>
> Subject: Re: [XS] JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent
> 
> 
> 
> On 16/05/2017 10:04, Baesken, Matthias wrote:
>> Hello, could you please review this small change :
>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8180413/
>>
>> it fixes a number of places in   jdk.jdwp.agent   where in case of an error 
>> it is attempted to write to NULL .
>>
>> Bug :  JDK-8180413 : avoid accessing NULL in jdk.jdwp.agent
>>
>>
>> https://bugs.openjdk.java.net/browse/JDK-8180413
>>
>>
> Can you bring this to serviceability-dev as that is the mailing list 
> where the JDWP agent is maintained?
> 
> -Alan
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(XS)(10): 8175342: assert(InstanceKlass::cast(k)->is_initialized()) failed: need to increase java_thread_min_stack_allowed calculation

[Dmitry Samersoff]

Chris,

Looks good to me.

-Dmitry

On 2017-03-17 10:31, Chris Plummer wrote:
> I should have been more clear. I need one more "reviewer", not another
> review from David.
> 
> thanks,
> 
> Chris
> 
> On 3/17/17 12:30 AM, Chris Plummer wrote:
>> Thanks for the review, David.
>>
>> I could still use one more review. Here's an updated webrev.
>>
>> http://cr.openjdk.java.net/~cjplummer/8175342/webrev.01/webrev.jdk
>>
>> cheers,
>>
>> Chris
>>
>> On 3/15/17 10:14 PM, David Holmes wrote:
>>> Hi Chris,
>>>
>>> On 16/03/2017 2:57 PM, Chris Plummer wrote:
>>>> Hello,
>>>>
>>>> Please review the following:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8175342
>>>> http://cr.openjdk.java.net/~cjplummer/8175342/webrev.00/webrev.jdk
>>>
>>> I think you want to disable the guardpage always, not just when a
>>> specific stack size is requested. You might not miss 64KB in 8MB but
>>> logically the guard page is never needed.
>>>
>>> Thanks,
>>> David
>>> -
>>>
>>>> The assert is somewhat misleading, although it did properly detect a
>>>> "too small" stack issue. The test was run with -Xss256k on a system
>>>> with
>>>> 64k pages. On this system 256k is suppose to be the smallest allowed
>>>> stack size, so -Xss256k should work. The thread that the assert happens
>>>> on is the main thread created by ContinueInNewThread0(). By default
>>>> pthread gives new threads a guard page the size of an OS page. pthreads
>>>> is suppose to add additional stack space for the guard page, but it
>>>> doesn't. Later we call current_stack_region(), which among other
>>>> things,
>>>> computes the size of the stack. It has the following code to deal with
>>>> the pthread guard page bug:
>>>>
>>>> // Work around NPTL stack guard error.
>>>> size_t guard_size = 0;
>>>> rslt = pthread_attr_getguardsize(, _size);
>>>> *bottom += guard_size;
>>>> *size   -= guard_size;
>>>>
>>>> So the net effect is hotspot treats the stack as only being 192k, not
>>>> 256k. However, in terms of usable stack space, hotspot then also
>>>> subtracts the red, yellow, and shadow zones. Each of these is one OS
>>>> page. So that subtracts another 192k, leaving us with 0k. The assert is
>>>> a by product of this.
>>>>
>>>> It turns out this pthread guard page problem is already fixed for all
>>>> java threads except the main thread. We do the following in
>>>> os::create_thread():
>>>>
>>>>   pthread_attr_setguardsize(,
>>>> os::Linux::default_guard_size(thr_type));
>>>>
>>>> For java threads, os::Linux::default_guard_size() returns 0, so the
>>>> above code removes the guard page for java threads. The fix I'm
>>>> proposing for the main thread does the same.
>>>>
>>>> Tested by running the test in question dozens of times on all supported
>>>> platforms. Also ran most tests we do for nightlies except for longer
>>>> running ones.
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR 8169115, java/net/InetAddress/ptr/lookup.sh failed intermittently

[Dmitry Samersoff]

Felix,

Changes looks good to me. Thank you for doing it.

-Dmitry


On 2016-12-08 13:35, Felix Yang wrote:
> Hi Dmitry,
> 
>I tested your suggested "icann.org" and it indeed works well. Please
> review the updated webrev:
> 
> http://cr.openjdk.java.net/~xiaofeya/8169115/webrev.02/
> 
> Thanks,
> Felix
> On 2016/12/6 19:16, Dmitry Samersoff wrote:
>> Felix,
>>
>> 1. I'm not sure that javaweb.sfbay.sun.com is the best domain name for
>> this test. Could we use different one (e.g. icann.org)
>>
>> 2. This test run JTREG -> Test VM -> Another VM. Could we optimize
>> process usage?
>>
>> It might be better to create a jtreg "same vm" process that
>>
>> 1. launch another process with -Djava.net.preferIPv4Stack=true
>> that do A and PRT lookup in one run.
>>
>> 2. Read results of process above, do PTR lookup with default
>> settings and compare results.
>>
>> -Dmitry
>>
>>
>> On 2016-12-06 12:06, Felix Yang wrote:
>>> Hi,
>>>
>>> please review the following patch. It generally coverts codes from
>>> shell to plain java.
>>>
>>> Bug:
>>>
>>>  https://bugs.openjdk.java.net/browse/JDK-8169115
>>>
>>> Webrev:
>>>
>>>  http://cr.openjdk.java.net/~xiaofeya/8169115/webrev.00/
>>>
>>> Thanks,
>>>
>>> Felix
>>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.

[Dmitry Samersoff]

Goetz,

This version of serviceability changes looks good to me.

-Dmitry

On 2016-12-07 18:26, Lindenmaier, Goetz wrote:
> Hi Dmitry, 
> 
> yes, new_jvmpath is consistent with the other variables. 
> I also merged the ifs in SDE.c.
> 
> new webrev:
> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.03/
> 
> Best regards,
>   Goetz.
> 
>> -----Original Message-
>> From: Dmitry Samersoff [mailto:dmitry.samers...@oracle.com]
>> Sent: Wednesday, December 07, 2016 2:43 PM
>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Java Core Libs
>> <core-libs-dev@openjdk.java.net>; serviceability-dev (serviceability-
>> d...@openjdk.java.net) <serviceability-...@openjdk.java.net>
>> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty
>> coding.
>>
>> Goetz,
>>
>> SDE.c:
>>
>> You might combine if at ll. 260 and 263 to one but it's just matter of test.
>>
>>  if (sti == baseStratumIndex || sti < 0) {
>> return; /* Java stratum - return unchanged */
>>  }
>>
>>> I'm not sure what you mean. I tried to fix it, but please
>>> double-check the new webrev.
>>
>> if cnt is <= 0 loop at l.267
>>
>> for (; cnt-- > 0; ++fromEntry) {
>>
>> is never run and we effectively do
>>
>>  *entryCountPtr = 0;
>>
>> at l.283
>>
>> So if we you suspect that cnt may become negative or 0:
>> (your v.01 changes)
>>
>>  260 if (sti < 0 && cnt > 0) {
>>  261 return;
>>  262 }
>>
>> it's better to check it early.
>>
>> But I'm not sure we have to care about negative/zero cnt here.
>>
>> -Dmitry
>>
>> On 2016-12-07 11:37, Lindenmaier, Goetz wrote:
>>> Hi Dmitry,
>>>
>>> thanks for looking at my change!
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.02
>>>
>>>> * src/java.base/unix/native/libjli/java_md_solinux.c
>>>> Is this line correct?
>>>> 519 jvmpath = JLI_StringDup(jvmpath);
>>>
>>> It seems pointless. Should I remove it?  (The whole file is a horror.)
>>>
>>>> * src/jdk.jdwp.agent/share/native/libjdwp/SDE.c
>>>> It might be better to return immediately if cnt < 0,
>>>> regardless of value of sti.
>>>
>>> I'm not sure what you mean. I tried to fix it, but please
>>> double-check the new webrev.
>>>
>>>> * src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c
>>>> It might be better to write
>>>>   arg.l_linger = (on) ? (unsigned short)value.i : 0;
>>>> and leave one copy of setsockopt() call
>>>
>>> Yes, looks better.
>>>
>>> Best regards,
>>>   Goetz
>>>
>>>
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2016-12-06 16:12, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>>
>>>>> This change fixes some minor issues found in our code scans.
>>>>>
>>>>> I hope this correctly addresses corelib and serviceability issues.
>>>>>
>>>>>
>>>>>
>>>>> Please review:
>>>>>
>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-
>> corlib_s11y/webrev.01/
>>>>>
>>>>>
>>>>>
>>>>> Best regards,
>>>>>
>>>>>   Goetz.
>>>>>
>>>>>
>>>>>
>>>>> Changes in detail:
>>>>>
>>>>>
>>>>>
>>>>> e_asin.c
>>>>>
>>>>> Code scan reports missing {}.
>>>>>
>>>>>
>>>>> The code "if (huge+x>one) {" is only there to set the inexact flag of
>>>>> the processor.
>>>>> It's a way to avoid the C compiler to optimize the code away. It is
>>>>> always true,
>>>>> so the parenthesis of the outer else don't matter.
>>>>>
>>>>> Although this basically just adds the {} I would like to submit this to
>>>>>
>>>>> avoid anybody else spends more the 30sec on understanding these
>>>>>
>>>>> if statements.
>>>>>
>>>>>
>>>>> k_standard.c
>>>>>
>>>>> exc.retval is returned below and thus should always be initialized.
>>>>>
>>>>>
>>>>> imageDecompressor.cpp
>>>>>
>>>>> Wrong destructor is used.  Reported also by David CARLIER
>>>>>
>>>>>
>>>>> java.c
>>>>>
>>>>> in line 1865 'name' was used, it should be 'alias'.
>>>>>
>>>>>
>>>>> java_md_solinux.c
>>>>>
>>>>> "//" in path is useless. Further down a free is missing.
>>>>>
>>>>>
>>>>> SDE.c
>>>>>
>>>>> Call to stratumTableIndex can return negative value if defaultStratumId
>>>>> == null.
>>>>>
>>>>>
>>>>> socket_md.c
>>>>>
>>>>> arg.l_linger is passed to setsockopt uninitialized. Its use is hidden in
>>>>> the macros.
>>>>>
>>>>>
>>>>> unpack.cpp
>>>>>
>>>>> n.slice should not get negative argument for end, which is passed from
>>>>> dollar1.
>>>>> But dollar1 can get negative where it is set to the result of
>>>>> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1).
>>>>>
>>>>
>>>>
>>>> --
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the sources.
>>
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.

[Dmitry Samersoff]

Goetz,

SDE.c:

You might combine if at ll. 260 and 263 to one but it's just matter of test.

 if (sti == baseStratumIndex || sti < 0) {
return; /* Java stratum - return unchanged */
 }

> I'm not sure what you mean. I tried to fix it, but please
> double-check the new webrev.

if cnt is <= 0 loop at l.267

for (; cnt-- > 0; ++fromEntry) {

is never run and we effectively do

 *entryCountPtr = 0;

at l.283

So if we you suspect that cnt may become negative or 0:
(your v.01 changes)

 260 if (sti < 0 && cnt > 0) {
 261 return;
 262 }

it's better to check it early.

But I'm not sure we have to care about negative/zero cnt here.

-Dmitry

On 2016-12-07 11:37, Lindenmaier, Goetz wrote:
> Hi Dmitry,
> 
> thanks for looking at my change!
> Updated webrev:
> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.02
> 
>> * src/java.base/unix/native/libjli/java_md_solinux.c
>> Is this line correct?
>> 519 jvmpath = JLI_StringDup(jvmpath);
> 
> It seems pointless. Should I remove it?  (The whole file is a horror.)
>  
>> * src/jdk.jdwp.agent/share/native/libjdwp/SDE.c
>> It might be better to return immediately if cnt < 0,
>> regardless of value of sti.
> 
> I'm not sure what you mean. I tried to fix it, but please 
> double-check the new webrev.
> 
>> * src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c
>> It might be better to write
>>   arg.l_linger = (on) ? (unsigned short)value.i : 0;
>> and leave one copy of setsockopt() call
> 
> Yes, looks better.
> 
> Best regards,
>   Goetz
> 
> 
>>
>> -Dmitry
>>
>>
>> On 2016-12-06 16:12, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>>
>>>
>>> This change fixes some minor issues found in our code scans.
>>>
>>> I hope this correctly addresses corelib and serviceability issues.
>>>
>>>
>>>
>>> Please review:
>>>
>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.01/
>>>
>>>
>>>
>>> Best regards,
>>>
>>>   Goetz.
>>>
>>>
>>>
>>> Changes in detail:
>>>
>>>
>>>
>>> e_asin.c
>>>
>>> Code scan reports missing {}.
>>>
>>>
>>> The code "if (huge+x>one) {" is only there to set the inexact flag of
>>> the processor.
>>> It's a way to avoid the C compiler to optimize the code away. It is
>>> always true,
>>> so the parenthesis of the outer else don't matter.
>>>
>>> Although this basically just adds the {} I would like to submit this to
>>>
>>> avoid anybody else spends more the 30sec on understanding these
>>>
>>> if statements.
>>>
>>>
>>> k_standard.c
>>>
>>> exc.retval is returned below and thus should always be initialized.
>>>
>>>
>>> imageDecompressor.cpp
>>>
>>> Wrong destructor is used.  Reported also by David CARLIER
>>>
>>>
>>> java.c
>>>
>>> in line 1865 'name' was used, it should be 'alias'.
>>>
>>>
>>> java_md_solinux.c
>>>
>>> "//" in path is useless. Further down a free is missing.
>>>
>>>
>>> SDE.c
>>>
>>> Call to stratumTableIndex can return negative value if defaultStratumId
>>> == null.
>>>
>>>
>>> socket_md.c
>>>
>>> arg.l_linger is passed to setsockopt uninitialized. Its use is hidden in
>>> the macros.
>>>
>>>
>>> unpack.cpp
>>>
>>> n.slice should not get negative argument for end, which is passed from
>>> dollar1.
>>> But dollar1 can get negative where it is set to the result of
>>> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1).
>>>
>>
>>
>> --
>> Dmitry Samersoff
>> Oracle Java development team, Saint Petersburg, Russia
>> * I would love to change the world, but they won't give me the sources.


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.

[Dmitry Samersoff]

Goetz,

On 2016-12-07 14:54, Lindenmaier, Goetz wrote:
> Ah, you're right, the 'lastslash' assignment!

In this case we have to keep strdup.

Could we at least change it to something like

 new_jvmpath = JLI_StringDup(jvmpath);

and use new_jvmpath below.

And yes, the whole file is a horror.

-Dmitry

> 
> Thanks,
>   Goetz.
> 
>> -Original Message-
>> From: David Holmes [mailto:david.hol...@oracle.com]
>> Sent: Wednesday, December 07, 2016 10:32 AM
>> To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; 'Dmitry Samersoff'
>> <dmitry.samers...@oracle.com>; Java Core Libs > d...@openjdk.java.net>; serviceability-dev (serviceability-
>> d...@openjdk.java.net) <serviceability-...@openjdk.java.net>
>> Subject: Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty
>> coding.
>>
>> On 7/12/2016 6:37 PM, Lindenmaier, Goetz wrote:
>>> Hi Dmitry,
>>>
>>> thanks for looking at my change!
>>> Updated webrev:
>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.02
>>>
>>>> * src/java.base/unix/native/libjli/java_md_solinux.c
>>>> Is this line correct?
>>>> 519 jvmpath = JLI_StringDup(jvmpath);
>>>
>>> It seems pointless. Should I remove it?  (The whole file is a horror.)
>>
>> Seems to me it is making a copy of the incoming char[] so that it can be
>> modified in this function without affecting the original char[] in the
>> caller.
>>
>> David
>> -
>>
>>
>>>> * src/jdk.jdwp.agent/share/native/libjdwp/SDE.c
>>>> It might be better to return immediately if cnt < 0,
>>>> regardless of value of sti.
>>>
>>> I'm not sure what you mean. I tried to fix it, but please
>>> double-check the new webrev.
>>>
>>>> * src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c
>>>> It might be better to write
>>>>   arg.l_linger = (on) ? (unsigned short)value.i : 0;
>>>> and leave one copy of setsockopt() call
>>>
>>> Yes, looks better.
>>>
>>> Best regards,
>>>   Goetz
>>>
>>>
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2016-12-06 16:12, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>>
>>>>> This change fixes some minor issues found in our code scans.
>>>>>
>>>>> I hope this correctly addresses corelib and serviceability issues.
>>>>>
>>>>>
>>>>>
>>>>> Please review:
>>>>>
>>>>> http://cr.openjdk.java.net/~goetz/wr16/8170663-
>> corlib_s11y/webrev.01/
>>>>>
>>>>>
>>>>>
>>>>> Best regards,
>>>>>
>>>>>   Goetz.
>>>>>
>>>>>
>>>>>
>>>>> Changes in detail:
>>>>>
>>>>>
>>>>>
>>>>> e_asin.c
>>>>>
>>>>> Code scan reports missing {}.
>>>>>
>>>>>
>>>>> The code "if (huge+x>one) {" is only there to set the inexact flag of
>>>>> the processor.
>>>>> It's a way to avoid the C compiler to optimize the code away. It is
>>>>> always true,
>>>>> so the parenthesis of the outer else don't matter.
>>>>>
>>>>> Although this basically just adds the {} I would like to submit this to
>>>>>
>>>>> avoid anybody else spends more the 30sec on understanding these
>>>>>
>>>>> if statements.
>>>>>
>>>>>
>>>>> k_standard.c
>>>>>
>>>>> exc.retval is returned below and thus should always be initialized.
>>>>>
>>>>>
>>>>> imageDecompressor.cpp
>>>>>
>>>>> Wrong destructor is used.  Reported also by David CARLIER
>>>>>
>>>>>
>>>>> java.c
>>>>>
>>>>> in line 1865 'name' was used, it should be 'alias'.
>>>>>
>>>>>
>>>>> java_md_solinux.c
>>>>>
>>>>> "//" in path is useless. Further down a free is missing.
>>>>>
>>>>>
>>>>> SDE.c
>>>>>
>>>>> Call to stratumTableIndex can return negative value if defaultStratumId
>>>>> == null.
>>>>>
>>>>>
>>>>> socket_md.c
>>>>>
>>>>> arg.l_linger is passed to setsockopt uninitialized. Its use is hidden in
>>>>> the macros.
>>>>>
>>>>>
>>>>> unpack.cpp
>>>>>
>>>>> n.slice should not get negative argument for end, which is passed from
>>>>> dollar1.
>>>>> But dollar1 can get negative where it is set to the result of
>>>>> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1).
>>>>>
>>>>
>>>>
>>>> --
>>>> Dmitry Samersoff
>>>> Oracle Java development team, Saint Petersburg, Russia
>>>> * I would love to change the world, but they won't give me the sources.


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M): 8170663: Fix minor issues in corelib and servicabilty coding.

[Dmitry Samersoff]

Goetz,

For serviceability code, please see comments below:

* src/java.base/share/native/libjli/java.c

No comments

* src/java.base/unix/native/libjli/java_md_solinux.c

Is this line correct?

519 jvmpath = JLI_StringDup(jvmpath);

* src/jdk.jdwp.agent/share/native/libjdwp/SDE.c

It might be better to return immediately if cnt < 0,
regardless of value of sti.

* src/jdk.jdwp.agent/unix/native/libdt_socket/socket_md.c

It might be better to write

  arg.l_linger = (on) ? (unsigned short)value.i : 0;

and leave one copy of setsockopt() call

-Dmitry


On 2016-12-06 16:12, Lindenmaier, Goetz wrote:
> Hi,
> 
>  
> 
> This change fixes some minor issues found in our code scans.
> 
> I hope this correctly addresses corelib and serviceability issues.
> 
>  
> 
> Please review:
> 
> http://cr.openjdk.java.net/~goetz/wr16/8170663-corlib_s11y/webrev.01/
> 
>  
> 
> Best regards,
> 
>   Goetz.
> 
>  
> 
> Changes in detail:
> 
>  
> 
> e_asin.c
> 
> Code scan reports missing {}.
> 
> 
> The code "if (huge+x>one) {" is only there to set the inexact flag of
> the processor.
> It's a way to avoid the C compiler to optimize the code away. It is
> always true,
> so the parenthesis of the outer else don't matter.
> 
> Although this basically just adds the {} I would like to submit this to
> 
> avoid anybody else spends more the 30sec on understanding these
> 
> if statements.
> 
> 
> k_standard.c
> 
> exc.retval is returned below and thus should always be initialized.
> 
> 
> imageDecompressor.cpp
> 
> Wrong destructor is used.  Reported also by David CARLIER
> 
> 
> java.c
> 
> in line 1865 'name' was used, it should be 'alias'.
> 
> 
> java_md_solinux.c
> 
> "//" in path is useless. Further down a free is missing.
> 
> 
> SDE.c
> 
> Call to stratumTableIndex can return negative value if defaultStratumId
> == null.
> 
> 
> socket_md.c
> 
> arg.l_linger is passed to setsockopt uninitialized. Its use is hidden in
> the macros.
> 
> 
> unpack.cpp
> 
> n.slice should not get negative argument for end, which is passed from
> dollar1.
> But dollar1 can get negative where it is set to the result of
> lastIndexOf(DOLLAR_MIN, DOLLAR_MAX, n, dollar2 - 1).
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR 8169115, java/net/InetAddress/ptr/lookup.sh failed intermittently

[Dmitry Samersoff]

Felix,

1. I'm not sure that javaweb.sfbay.sun.com is the best domain name for
this test. Could we use different one (e.g. icann.org)

2. This test run JTREG -> Test VM -> Another VM. Could we optimize
process usage?

It might be better to create a jtreg "same vm" process that

   1. launch another process with -Djava.net.preferIPv4Stack=true
   that do A and PRT lookup in one run.

   2. Read results of process above, do PTR lookup with default
   settings and compare results.

-Dmitry


On 2016-12-06 12:06, Felix Yang wrote:
> Hi,
> 
>please review the following patch. It generally coverts codes from
> shell to plain java.
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8169115
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~xiaofeya/8169115/webrev.00/
> 
> Thanks,
> 
> Felix
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR 8166501 : compilation error in stackwalk.cpp on some gccs

[Dmitry Samersoff]

Brent,

Looks good for me.

-Dmitry

On 2016-09-22 19:04, Brent Christian wrote:
> Hi,
> 
> Looks like my 8165372 change broke the slowdebug build.  Please review
> my fix (which also breaks up a pretty long line):
> 
> --- a/src/share/vm/prims/stackwalk.cpp
> +++ b/src/share/vm/prims/stackwalk.cpp
> @@ -331,10 +331,12 @@
>  assert (use_frames_array(mode), "Bad mode for get live frame");
>  RegisterMap regMap(jt, true);
>  LiveFrameStream stream(jt, );
> -return fetchFirstBatch(stream, stackStream, mode, skip_frames,
> frame_count, start_index, frames_array, CHECK_NULL);
> +return fetchFirstBatch(stream, stackStream, mode, skip_frames,
> frame_count,
> +   start_index, frames_array, THREAD);
>} else {
>  JavaFrameStream stream(jt, mode);
> -return fetchFirstBatch(stream, stackStream, mode, skip_frames,
> frame_count, start_index, frames_array, CHECK_NULL);
> +return fetchFirstBatch(stream, stackStream, mode, skip_frames,
> frame_count,
> +   start_index, frames_array, THREAD);
>}
>  }
> 
> Thanks!
> -Brent
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S) : 8166262 : failurehandler should not use jtreg internal API

[Dmitry Samersoff]

Igor,

Looks good for me.

-Dmitry

On 2016-09-19 12:17, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev/8166262/webrev.00/
>> 62 lines changed: 56 ins; 2 del; 4 mod;
> 
> Hi all,
> 
> could you please review this small patch which removes usage of jtreg 
> internal API from failurehandler lib?
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8166262
> webrev: http://cr.openjdk.java.net/~iignatyev/8166262/webrev.00/
> 
> Thanks,
> — Igor
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: 8166189: Fix for Bug 8165524 breaks AIX build

[Dmitry Samersoff]

Christoph,

java_md_aix.c

32: missed comma after L_GETINFO

Should you return an error from fill_dll_info rather than print it to
stderr?

-Dmitry



On 2016-09-16 12:58, Langer, Christoph wrote:
> Hi,
> 
> the fix for https://bugs.openjdk.java.net/browse/JDK-8165524 breaks the AIX 
> build as function dladdr is not available on AIX.
> 
> There already exist ports of that API in hotspot and awt. With the proposed 
> change I duplicate the awt port to libjli. This is maybe only a quick fix - 
> eventually we should think about consolidating and using the hotspot port in 
> all places by exporting it from libjvm.so for AIX.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8166189
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8166189.0/
> 
> Thanks
> Christoph
> 
> 
>> -Original Message-
>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
>> Of Kumar Srinivasan
>> Sent: Montag, 12. September 2016 22:57
>> To: core-libs-dev <core-libs-dev@openjdk.java.net>; Mandy Chung
>> <mandy.ch...@oracle.com>; Chris Bensen <chris.ben...@oracle.com>
>> Subject: RFR: 8165524: Better detect JRE that Linux JLI will be using
>>
>> Hello,
>>
>> I am sponsoring this changeset for Chris Bensen of the java packager
>> team, please review  fix for the launcher to  better locate java.home.
>>
>> http://cr.openjdk.java.net/~ksrini/8165524/
>>
>> Thanks
>> Kumar
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8161360,,Deprecated vfork() should not be used on Solaris

[Dmitry Samersoff]

Alan,

Changes looks good for me.

-Dmitry

On 2016-08-31 14:55, Alan Burlison wrote:
> vfork(2) is deprecated on Solaris and using it generates compiler
> warnings. When compiled with warnings-as-errors, this results in
> compilation failures.
> 
> Bug:https://bugs.openjdk.java.net/browse/JDK-8161360
> Webrev: http://cr.openjdk.java.net/~alanbur/JDK-8161360
> 
> Thanks,
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8161360, , Deprecated vfork() should not be used on Solaris

[Dmitry Samersoff]

Martin,

Valid launch mechanisms for Solaris set in ProcessImpl.java as:

  SOLARIS(LaunchMechanism.POSIX_SPAWN, LaunchMechanism.FORK),

So it's not possible to set VFORK as LaunchMechanism for Solaris.
and this fix doesn't change anything from customer perspective.

-Dmitry


On 2016-09-01 04:55, Martin Buchholz wrote:
> Does an attempt to use vfork on Solaris result in something reasonable like
> UnsupportedOperationException?
> 
> On Wed, Aug 31, 2016 at 4:55 AM, Alan Burlison <alan.burli...@oracle.com>
> wrote:
> 
>> vfork(2) is deprecated on Solaris and using it generates compiler
>> warnings. When compiled with warnings-as-errors, this results in
>> compilation failures.
>>
>> Bug:https://bugs.openjdk.java.net/browse/JDK-8161360
>> Webrev: http://cr.openjdk.java.net/~alanbur/JDK-8161360
>>
>> Thanks,
>>
>> --
>> Alan Burlison
>> --
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDK 9 RFR of 8161970, 8157664: Remove 4 tools tests from ProblemList.txt

[Dmitry Samersoff]

Amy,

Looks good for me!

-Dmitry


On 2016-08-02 08:57, Amy Lu wrote:
> tools/jlink/JLinkOptimTest.java
> This test has been removed in JDK-8160829
> 
> sun/tools/jinfo/JInfoSanityTest.java
> sun/tools/jinfo/JInfoRunningProcessFlagTest.java
> sun/tools/jmap/heapconfig/JMapHeapConfigTest.java
> These tests have been removed in JDK-8155091
> 
> Please review the patch to delete these tests from ProblemList.txt.
> 
> bug:
> https://bugs.openjdk.java.net/browse/JDK-8161970
> https://bugs.openjdk.java.net/browse/JDK-8157664
> 
> webrev: http://cr.openjdk.java.net/~amlu/8161970-8157664/webrev.00/
> 
> Thanks,
> Amy
> 
> --- old/test/ProblemList.txt  2016-08-02 13:44:34.0 +0800
> +++ new/test/ProblemList.txt  2016-08-02 13:44:34.0 +0800
> @@ -318,7 +318,7 @@
>  
>  
>  
> -# jdk_tools
> +# core_tools
>  
>  tools/pack200/CommandLineTests.java 
> 7143279,8059906 generic-all
>  
> @@ -368,20 +368,14 @@
>  
>  sun/tools/jcmd/TestJcmdSanity.java  8031482 
> windows-all
>  
> -sun/tools/jmap/heapconfig/JMapHeapConfigTest.java   
> 8072131,8132452 generic-all
> -
>  sun/tools/jstatd/TestJstatdExternalRegistry.java8046285 
> generic-all
>  
>  sun/tools/jps/TestJpsJar.java   8160923 
> generic-all
>  
>  sun/tools/jps/TestJpsJarRelative.java   6456333 
> generic-all
>  
> -sun/tools/jinfo/JInfoRunningProcessFlagTest.java6734748 
> generic-all
> -
>  sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java8057732 
> generic-all
>  
> -sun/tools/jinfo/JInfoSanityTest.java8059035 
> generic-all
> -
>  demo/jvmti/compiledMethodLoad/CompiledMethodLoadTest.java   8151899 
> generic-all
>  
>  
> @@ -391,9 +385,3 @@
>  com/sun/jndi/ldap/DeadSSLLdapTimeoutTest.java   8141370 
> linux-i586,macosx-all
>  
>  
> -
> -# core_tools
> -
> -tools/jlink/JLinkOptimTest.java 8159264 
> generic-all
> -
> -
> 
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR (XS): 8157188: 2 test failures in demo/jvmti due to unexpected class file version 53

[Dmitry Samersoff]

David,

Looks good for me.

Probably I was the last person who do something with java_crw_demo.c.

-Dmitry

On 2016-05-19 07:52, David Holmes wrote:
> Not sure who really owns this file so cc'ing core-libs and serviceability.
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8157188
> 
> The file src/java.base/share/native/include/classfile_constants.h
> describes information about classfiles and is used by libverify and
> ./demo/share/jvmti/java_crw_demo/java_crw_demo.c
> 
> This file has not been updated for classfile version 53 and so asserts
> will fail in java_crw_demo.c when it encounters classes compiled for
> version 53 - as they now are. This version change caused test failures
> in the hotspot forest when it was pulled down earlier this week.
> 
> This fix trivially bumps the current version number to 53 to fix the
> failing tests. It is a separate issue as to whether other changes are
> needed in this file to reflect what is new with classfile version 53.
> 
> Diff below.
> 
> Thanks,
> David
> --
> 
> diff -r 3eea6819cc1f
> src/java.base/share/native/include/classfile_constants.h
> --- a/src/java.base/share/native/include/classfile_constants.h
> +++ b/src/java.base/share/native/include/classfile_constants.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2004, 2012, Oracle and/or its affiliates. All rights
> reserved.
> + * Copyright (c) 2004, 2016, Oracle and/or its affiliates. All rights
> reserved.
>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>   *
>   * This code is free software; you can redistribute it and/or modify it
> @@ -31,7 +31,7 @@
>  #endif
> 
>  /* Classfile version number for this information */
> -#define JVM_CLASSFILE_MAJOR_VERSION 52
> +#define JVM_CLASSFILE_MAJOR_VERSION 53
>  #define JVM_CLASSFILE_MINOR_VERSION 0
> 
>  /* Flags */


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: (Round 2) RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

Looks good for me!

-Dmitry


On 2016-04-13 12:12, Thomas Stüfe wrote:
> Hi Roger, Dmitry,
> 
> May I ask you both to have a last look at this change before I commit?
> It took a while for this to go through our internal tests, hence the delay.
> 
> New
> version: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.03/webrev/
> Delta to last
> version: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02-webrev.03/webrev/
> 
> The changes are mostly cosmetic:
> 
> - I tweaked a number of comments to make them clearer
> - If getrlimit(RLIMIT_NOFILE) returns an error, I now handle this correctly.
> - Just for readability, I explicitly initialize global variables instead
> of relying on static zero-initialization.
> - As Roger requested, I changed accesses to the entry table elements
> from using implicit pointer arithmetic to explicit array accesses with "&".
> 
> Thank you for your time!
> 
> Kind Regards, Thomas
> 
> On Sat, Mar 12, 2016 at 8:38 AM, Thomas Stüfe <thomas.stu...@gmail.com
> <mailto:thomas.stu...@gmail.com>> wrote:
> 
> Thank you Roger!
> 
> On Fri, Mar 11, 2016 at 4:26 PM, Roger Riggs <roger.ri...@oracle.com
> <mailto:roger.ri...@oracle.com>> wrote:
> 
> Hi Thomas,
> 
> When returning the address of the fdentry, the style using
> [fd] is preferred over
> the implicit pointer arithmetic (as it was in the previous version).
> 
> Occurs in all 3 deltas:
> 
> src/java.base/*/native/libnet/*_close.c:
> +result = fdTable + fd;
> 
> and
> +result = fdOverflowTable[rootindex] + slabindex;
> 
> The rest looks fine.
> 
> Thanks, Roger
> 
> 
> 
> 
> On 3/10/2016 7:59 AM, Thomas Stüfe wrote:
> 
> Thank you Dmitry!
> 
> I will fix the typo before comitting.
> 
> Kind Regards, Thomas
> 
> On Thu, Mar 10, 2016 at 9:19 AM, Dmitry Samersoff <
> dmitry.samers...@oracle.com
> <mailto:dmitry.samers...@oracle.com>> wrote:
> 
> Thomas,
> 
> Looks good for me. But please wait for someone from
> core-libs team.
> 
> PS: Could you also fix a typeo at 79, 51, 53?
> 
>  s/initialized/initialization/
> 
>   51  * Heap allocated during initialization - one entry
> per fd
> 
> -Dmitry
> 
> On 2016-03-10 10:59, Thomas Stüfe wrote:
> 
> Hi,
> 
> may I have a review of this new iteration for this fix?
> 
> Thank you!
> 
> Thomas
> 
> On Thu, Mar 3, 2016 at 5:26 PM, Thomas Stüfe
> <thomas.stu...@gmail.com
> <mailto:thomas.stu...@gmail.com>>
> wrote:
> 
> Hi all,
> 
> https://bugs.openjdk.java.net/browse/JDK-8150460
> 
> thanks to all who took the time to review the
> first version of this fix!
> 
> This is the new version:
> 
> 
> 
> 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/
> 
> I reworked the fix, trying to add in all the
> input I got: This fix uses
> 
> a
> 
> simple one-dimensional array, preallocated at
> startup, for low-value
> 
> file
> 
> descriptors. Like the code did before. Only for
> large values of file
> descriptors it switches to an overflow table,
> organized as two
> 
> dimensional
> 
> sparse array of fixed sized slabs, which are
> allocated on demand. Only
> 
> the
> 
> overflow table is protected by a lock.
> 
> For 99% of all cases we will be using the plain
> simple fdTable structure
> as before. Only for unusually large file
> descriptor values we will be
> 
> using
> 
> this overflow table.
> 
>

Re: (Round 2) RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

Looks good for me. But please wait for someone from core-libs team.

PS: Could you also fix a typeo at 79, 51, 53?

s/initialized/initialization/

 51  * Heap allocated during initialization - one entry per fd

-Dmitry

On 2016-03-10 10:59, Thomas Stüfe wrote:
> Hi,
> 
> may I have a review of this new iteration for this fix?
> 
> Thank you!
> 
> Thomas
> 
> On Thu, Mar 3, 2016 at 5:26 PM, Thomas Stüfe <thomas.stu...@gmail.com>
> wrote:
> 
>> Hi all,
>>
>> https://bugs.openjdk.java.net/browse/JDK-8150460
>>
>> thanks to all who took the time to review the first version of this fix!
>>
>> This is the new version:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/
>>
>> I reworked the fix, trying to add in all the input I got: This fix uses a
>> simple one-dimensional array, preallocated at startup, for low-value file
>> descriptors. Like the code did before. Only for large values of file
>> descriptors it switches to an overflow table, organized as two dimensional
>> sparse array of fixed sized slabs, which are allocated on demand. Only the
>> overflow table is protected by a lock.
>>
>> For 99% of all cases we will be using the plain simple fdTable structure
>> as before. Only for unusually large file descriptor values we will be using
>> this overflow table.
>>
>> Memory footprint is kept low: for small values of RLIMIT_NOFILE, we will
>> only allocate as much space as we need. Only if file descriptor values get
>> large, memory is allocated in the overflow table.
>>
>> Note that I avoided the proposed double-checked locking solution: I find
>> it too risky in this place and also unnecessary. When calling getFdEntry(),
>> we will be executing a blocking IO operation afterwards, flanked by two
>> mutex locks (in startOp and endOp). So, I do not think the third mutex lock
>> in getFdEntry will add much, especially since it is only used in case of
>> larger file descriptor values.
>>
>> I also added the fix to bsd_close.c and aix_close.c. I do not like this
>> code triplication. I briefly played around with unifying this code, but
>> this is more difficult than it seems: implementations subtly differ between
>> the three platforms, and solaris implementation is completely different. It
>> may be a worthwhile cleanup, but that would be a separate issue.
>>
>> I did some artificial tests to check how the code does with many and large
>> file descriptor values, all seemed to work well. I also ran java/net jtreg
>> tests on Linux and AIX.
>>
>> Kind Regards, Thomas
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

> The more I look at this, the more I think that the costs for a
> pthread mutex lock are acceptable in this case: we are about to do a
> blocking IO operation anyway, which is already flanked by two mutex
> locking calls (in startOp and endOp). I doubt that a third mutex call
> will be the one making the costs suddenly unacceptable. Especially
> since they can be avoided altogether for low value mutex numbers (the
> optimization Roger suggested).

After closer look to the code in a whole - I agree with you.

-Dmitry


On 2016-03-03 12:32, Thomas Stüfe wrote:
> Hi Hans,
> 
> On Thu, Mar 3, 2016 at 4:08 AM, Hans Boehm <hbo...@google.com 
> <mailto:hbo...@google.com>> wrote:
> 
> 
> On Wed, Mar 2, 2016 at 12:09 AM, Thomas Stüfe 
> <thomas.stu...@gmail.com <mailto:thomas.stu...@gmail.com>> wrote:
>> 
>> Hi Hans,
>> 
>> thanks for the hint!
>> 
>> But how would I do this for my problem:
>> 
>> Allocate memory, zero it out and then store the pointer into a
>> variable seen by other threads, while preventing the other threads
>> from seeing . I do not understand how atomics would help: I can
>> make the pointer itself an atomic, but that only guarantees memory
>> ordering in regard to this variable, not to the allocated memory.
>> 
>> Kind Regards, Thomas
> 
> C11 atomics work essentially like Java volatiles: They order other 
> memory accesses as well.  If you declare the pointer to be atomic, 
> and store into it, then another thread reading the newly assigned 
> value will also see the stores preceding the pointer store.  Since 
> the pointer is the only value that can be accessed concurrently by 
> multiple threads (with not all accesses reads), it's the only object 
> that needs to be atomic.  In this case, it's sufficient to store into
> the pointer with
> 
> atomic_store_explicit(, , memory_order_release);
> 
> and read it with
> 
> atomic_load_explicit(, memory_order_acquire);
> 
> which are a bit cheaper.
> 
> 
> However, this is C11 specific, and I don't know whether that's 
> acceptable to use in this context.
> 
> If you can't assume C11, the least incorrect workaround is generally 
> to make the pointer volatile, precede the store with a fence, and 
> follow the load with a fence.  On x86, both fences just need to 
> prevent compiler reordering.
> 
> 
> 
> Thank you for that excellent explanation!
> 
> This may be just my ignorance, but I actually did not know that
> atomics are now a part of the C standard. I took this occasion to
> look up all other C11 features and this is quite neat :) Nice to see
> that C continues to live.
> 
> I am very hesitant though about introducing C11 features into the
> JDK. We deal with notoriously oldish compilers, especially on AIX,
> and I do not want to be the first to force C11, especially not on
> such a side issue.
> 
> The more I look at this, the more I think that the costs for a
> pthread mutex lock are acceptable in this case: we are about to do a
> blocking IO operation anyway, which is already flanked by two mutex
> locking calls (in startOp and endOp). I doubt that a third mutex call
> will be the one making the costs suddenly unacceptable. Especially
> since they can be avoided altogether for low value mutex numbers (the
> optimization Roger suggested).
> 
> I will do some performance tests and check whether the added locking 
> calls are even measurable.
> 
> Thomas
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

> For fd < 65535, I effectively fall back to a plain array lookup by
> setting the size of the base table to 1. So, for this case the sparse
> array degenerates to a one-dimensional plain array.

It might be good to make it more explicit: just allocate a separate
array for values less than 65535 and skip other machinery if
nbr_files.rlim_max less than 65536.

But it's just a cosmetic, so feel free to leave the code as is.

-Dmitry



On 2016-03-01 16:33, Thomas Stüfe wrote:
> Hi Dmitry,
> 
> On Tue, Mar 1, 2016 at 1:44 PM, Dmitry Samersoff
> <dmitry.samers...@oracle.com <mailto:dmitry.samers...@oracle.com>> wrote:
> 
> Christoph,
> 
> > Dmitry, I think you are referring to an outdated version of the
> > webrev, the current one is this:
> 
> Yes. Sorry!
> 
> You may consider a bit different approach to save memory:
> 
> Allocate multiple baseTables for different ranges of fd's with
> plain array of 32 * (fdEntry_t*) for simple case.
> 
> i.e. if (fd < 32)
>  do plain array lookup
> 
>  if (fd < N1)
>  do two steps lookup in baseTable1
> 
>  if (fd < N2)
>  do two steps lookup in baseTable2
> 
> 
> How does this differ from my approach
> in 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/
>  ?
> 
> For fd < 65535, I effectively fall back to a plain array lookup by
> setting the size of the base table to 1. So, for this case the sparse
> array degenerates to a one-dimensional plain array.
> 
> Kind Regards, Thomas
> 
>  
> 
>  ...
> 
> -Dmitry
> 
> 
> 
> On 2016-03-01 13:47, Langer, Christoph wrote:
> > Hi Dmitry, Thomas,
> >
> > Dmitry, I think you are referring to an outdated version of the
> > webrev, the current one is this:
> >
> 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/
> >
> >  However, I agree - the lock should probably not be taken every time
> > but only in the case where we find the entry table was not yet
> > allocated.
> >
> > So, maybe getFdEntry should always do this: entryTable =
> > fdTable[rootArrayIndex]; // no matter if rootArrayIndex is 0
> >
> > Then check if entryTable is NULL and if yes then enter a guarded
> > section which does the allocation and before that checks if another
> > thread did it already.
> >
> > Also I'm wondering if the entryArrayMask and the rootArrayMask should
> > be calculated once in the init() function and stored in a static
> > field? Because right now it is calculated every time getFdEntry() is
> > called and I don't think this would be optimized by inlining...
> >
> > Best regards Christoph
> >
> > -Original Message- From: core-libs-dev
> > [mailto:core-libs-dev-boun...@openjdk.java.net
> <mailto:core-libs-dev-boun...@openjdk.java.net>] On Behalf Of Dmitry
> > Samersoff Sent: Dienstag, 1. März 2016 11:20 To: Thomas Stüfe
> > <thomas.stu...@gmail.com <mailto:thomas.stu...@gmail.com>>; Java
> Core Libs
> > <core-libs-dev@openjdk.java.net
> <mailto:core-libs-dev@openjdk.java.net>> Subject: Re: RFR(s): 8150460:
> > (linux|bsd|aix)_close.c: file descriptor table may become large or
> > may not work at all
> >
> > Thomas,
> >
> > Sorry for being later.
> >
> > I'm not sure we should take a lock at ll. 131 for each fdTable
> > lookup.
> >
> > As soon as we never deallocate fdTable[base_index] it's safe to try
> > to return value first and then take a slow path (take a lock and
> > check fdTable[base_index] again)
> >
> > -Dmitry
> >
> >
> > On 2016-02-24 20:30, Thomas Stüfe wrote:
> >> Hi all,
> >>
> >> please take a look at this proposed fix.
> >>
> >> The bug: https://bugs.openjdk.java.net/browse/JDK-8150460 The
> >> Webrev:
> >>
> 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.00/webrev/
> >>
> >>
> >>
> Basically, the file descriptor table implemented in linux_close.c
> may not
> >> work for RLIMIT_NO_FILE=infinite or may grow very large (I saw a
> >> 50MB table) for high values for RLIMIT_NO_FILE. Please see details
> >> in the bug

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

We probably can do:

if (fdTable[rootArrayIndex] != NULL) {
   entryTable = fdTable[rootArrayIndex];
}
else { // existing code
  pthread_mutex_lock();
  if (fdTable[rootArrayIndex] == NULL) {
  
  }
}

-Dmitry


On 2016-03-01 16:13, Thomas Stüfe wrote:
> Dmitry, Christoph,
> 
> I am not 100% sure this would work for weak ordering platforms.
> 
> If I understand you correctly you suggest the double checking pattern:
> 
> if (basetable[index] == NULL) {
> lock
> if (basetable[index] == NULL) {
> basetable[index] = calloc(size);
> }
>  unlock
> }
> 
> The problem I cannot wrap my head around is how to make this safe for
> all platforms. Note: I am not an expert for this. 
> 
> How do you prevent the "reading thread reads partially initialized
> object" problem?
> 
> Consider this: We need to allocate memory, set it completely to zero and
> then store a pointer to it in basetable[index]. This means we have
> multiple stores - one store for the pointer, n stores for zero-ing out
> the memory, and god knows how many stores the C-Runtime allcoator needs
> to update its internal structures. 
> 
> On weak ordering platforms like ppc (and arm?), the store for
> basetable[index] may be visible before the other stores, so the reading
> threads, running on different CPUs, may read a pointer to partially
> initialized memory. What you need is a memory barrier between the
> calloc() and store of basetable[index], to prevent the latter store from
> floating above the other stores.
> 
> I did not find anything about multithread safety in the calloc() docs,
> or guaranteed barrier behaviour, nor did I expect anything. In the
> hotspot we have our memory barrier APIs, but in the JDK I am confined to
> basic C and there is no way that I know of to do memory barriers with
> plain Posix APIs. 
> 
> Bottomline, I am not sure. Maybe I am too cautious here, but I do not
> see a way to make this safe without locking the reader thread too. 
> 
> Also, we are about to do an IO operation - is a mutex really that bad
> here? Especially with the optimization Roger suggested of pre-allocating
> the basetable[0] array and omitting lock protection there?
> 
> Kind Regards,
> 
> Thomas
> 
> 
> 
> 
> On Tue, Mar 1, 2016 at 11:47 AM, Langer, Christoph
> <christoph.lan...@sap.com <mailto:christoph.lan...@sap.com>> wrote:
> 
> Hi Dmitry, Thomas,
> 
> Dmitry, I think you are referring to an outdated version of the
> webrev, the current one is this:
> 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/
> 
> However, I agree - the lock should probably not be taken every time
> but only in the case where we find the entry table was not yet
> allocated.
> 
> So, maybe getFdEntry should always do this:
> entryTable = fdTable[rootArrayIndex]; // no matter if rootArrayIndex
> is 0
> 
> Then check if entryTable is NULL and if yes then enter a guarded
> section which does the allocation and before that checks if another
> thread did it already.
> 
> Also I'm wondering if the entryArrayMask and the rootArrayMask
> should be calculated once in the init() function and stored in a
> static field? Because right now it is calculated every time
> getFdEntry() is called and I don't think this would be optimized by
> inlining...
> 
> Best regards
> Christoph
> 
> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net
> <mailto:core-libs-dev-boun...@openjdk.java.net>] On Behalf Of Dmitry
> Samersoff
> Sent: Dienstag, 1. März 2016 11:20
> To: Thomas Stüfe <thomas.stu...@gmail.com
> <mailto:thomas.stu...@gmail.com>>; Java Core Libs
> <core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net>>
> Subject: Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file
> descriptor table may become large or may not work at all
> 
> Thomas,
> 
> Sorry for being later.
> 
> I'm not sure we should take a lock at ll. 131 for each fdTable lookup.
> 
> As soon as we never deallocate fdTable[base_index] it's safe to try to
> return value first and then take a slow path (take a lock and check
> fdTable[base_index] again)
> 
> -Dmitry
> 
> 
> On 2016-02-24 20:30, Thomas Stüfe wrote:
> > Hi all,
> >
> > please take a look at this proposed fix.
> >
> > The bug: https://bugs.openjdk.java.net/browse/JDK-8150460
> > The Webrev:
> >
> 
> http:/

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Christoph,

> Dmitry, I think you are referring to an outdated version of the
> webrev, the current one is this:

Yes. Sorry!

You may consider a bit different approach to save memory:

Allocate multiple baseTables for different ranges of fd's with
plain array of 32 * (fdEntry_t*) for simple case.

i.e. if (fd < 32)
 do plain array lookup

 if (fd < N1)
 do two steps lookup in baseTable1

 if (fd < N2)
 do two steps lookup in baseTable2

 ...

-Dmitry



On 2016-03-01 13:47, Langer, Christoph wrote:
> Hi Dmitry, Thomas,
> 
> Dmitry, I think you are referring to an outdated version of the
> webrev, the current one is this: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.01/webrev/
>
>  However, I agree - the lock should probably not be taken every time
> but only in the case where we find the entry table was not yet
> allocated.
> 
> So, maybe getFdEntry should always do this: entryTable =
> fdTable[rootArrayIndex]; // no matter if rootArrayIndex is 0
> 
> Then check if entryTable is NULL and if yes then enter a guarded
> section which does the allocation and before that checks if another
> thread did it already.
> 
> Also I'm wondering if the entryArrayMask and the rootArrayMask should
> be calculated once in the init() function and stored in a static
> field? Because right now it is calculated every time getFdEntry() is
> called and I don't think this would be optimized by inlining...
> 
> Best regards Christoph
> 
> -Original Message- From: core-libs-dev
> [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf Of Dmitry
> Samersoff Sent: Dienstag, 1. März 2016 11:20 To: Thomas Stüfe
> <thomas.stu...@gmail.com>; Java Core Libs
> <core-libs-dev@openjdk.java.net> Subject: Re: RFR(s): 8150460:
> (linux|bsd|aix)_close.c: file descriptor table may become large or
> may not work at all
> 
> Thomas,
> 
> Sorry for being later.
> 
> I'm not sure we should take a lock at ll. 131 for each fdTable
> lookup.
> 
> As soon as we never deallocate fdTable[base_index] it's safe to try
> to return value first and then take a slow path (take a lock and
> check fdTable[base_index] again)
> 
> -Dmitry
> 
> 
> On 2016-02-24 20:30, Thomas Stüfe wrote:
>> Hi all,
>> 
>> please take a look at this proposed fix.
>> 
>> The bug: https://bugs.openjdk.java.net/browse/JDK-8150460 The
>> Webrev: 
>> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.00/webrev/
>>
>>
>> 
Basically, the file descriptor table implemented in linux_close.c may not
>> work for RLIMIT_NO_FILE=infinite or may grow very large (I saw a
>> 50MB table) for high values for RLIMIT_NO_FILE. Please see details
>> in the bug description.
>> 
>> The proposed solution is to implement the file descriptor table not
>> as plain array, but as a twodimensional sparse array, which grows
>> on demand. This keeps the memory footprint small and fixes the
>> corner cases described in the bug description.
>> 
>> Please note that the implemented solution is kept simple, at the
>> cost of somewhat higher (some kb) memory footprint for low values
>> of RLIMIT_NO_FILE. This can be optimized, if we even think it is
>> worth the trouble.
>> 
>> Please also note that the proposed implementation now uses a mutex
>> lock for every call to getFdEntry() - I do not think this matters,
>> as this is all in preparation for an IO system call, which are
>> usually way more expensive than a pthread mutex. But again, this
>> could be optimized.
>> 
>> This is an implementation proposal for Linux; the same code found
>> its way to BSD and AIX. Should you approve of this fix, I will
>> modify those files too.
>> 
>> Thank you and Kind Regards, Thomas
>> 
> 
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

Sorry for being later.

I'm not sure we should take a lock at ll. 131 for each fdTable lookup.

As soon as we never deallocate fdTable[base_index] it's safe to try to
return value first and then take a slow path (take a lock and check
fdTable[base_index] again)

-Dmitry


On 2016-02-24 20:30, Thomas Stüfe wrote:
> Hi all,
> 
> please take a look at this proposed fix.
> 
> The bug: https://bugs.openjdk.java.net/browse/JDK-8150460
> The Webrev:
> http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.00/webrev/
> 
> Basically, the file descriptor table implemented in linux_close.c may not
> work for RLIMIT_NO_FILE=infinite or may grow very large (I saw a 50MB
> table) for high values for RLIMIT_NO_FILE. Please see details in the bug
> description.
> 
> The proposed solution is to implement the file descriptor table not as
> plain array, but as a twodimensional sparse array, which grows on demand.
> This keeps the memory footprint small and fixes the corner cases described
> in the bug description.
> 
> Please note that the implemented solution is kept simple, at the cost of
> somewhat higher (some kb) memory footprint for low values of RLIMIT_NO_FILE.
> This can be optimized, if we even think it is worth the trouble.
> 
> Please also note that the proposed implementation now uses a mutex lock for
> every call to getFdEntry() - I do not think this matters, as this is all in
> preparation for an IO system call, which are usually way more expensive
> than a pthread mutex. But again, this could be optimized.
> 
> This is an implementation proposal for Linux; the same code found its way
> to BSD and AIX. Should you approve of this fix, I will modify those files
> too.
> 
> Thank you and Kind Regards, Thomas
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR 4823133: RandomAccessFile.length() is not thread-safe

[Dmitry Samersoff]

Vyom,

I did some tests and the problem mentioned below is not relevant to
Java. I can't reproduce it with Java testcase.

Sorry for a noise.

-Dmitry

On 2015-12-18 23:42, vyom wrote:
> Hi Dmitry,
> 
> thanks for the review can you please explain little bit more, as per my
> testing and implementation i did not found any differences with fix and
> without fix. Even i checked the java.io.File.length() and there also it
> looks like we are using stat64().
> 
> as per you mail i truncate the file and with and without fix length is
> 102400, can you please explain little bit more about the problem that
> you mention it will be help full for me to debug further.
> 
> Thanks,
> Vyom
> 
> 
> On Friday 18 December 2015 05:35 PM, Dmitry Samersoff wrote:
>> Vyom,
>>
>> If I read the changes correctly, current code returns result of lseek()
>> but your code returns result of fstat().
>>
>> I'm not sure it's a correct replacement.
>>
>>
>> dooku:test#truncate --size=102400 test.me
>>
>> dooku:test#./test
>> STAT: 102400 0 Success
>> SEEK: 2 0 Success
>>
>> Moreover, if you truncate a file to value that large than available free
>> space, lseek returns appropriate error but stat - not.
>>
>> -Dmitry
>>
>>
>>
>> On 2015-12-16 11:56, vyom wrote:
>>> Hi All,
>>>
>>> Please find the updated
>>> webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.1/
>>> <http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.1/>). I
>>> incorporated the review comments by Roger Riggs.
>>>
>>> Thanks,
>>> Vyom
>>>
>>>
>>> On Tuesday 15 December 2015 10:01 PM, Roger Riggs wrote:
>>>> Hi Yvom,
>>>>
>>>> Minor comments:
>>>>
>>>> src/java.base/share/native/libjava/RandomAccessFile.c:
>>>>   - "length fail" might be clearer as "GetLength failed"
>>>>
>>>> src/java.base/unix/native/libjava/io_util_md.c:
>>>>
>>>>   - Please add a comment before the define of FILE_OFFSET_BITS to
>>>> indicate where it is used and why it is there.
>>>>   - BTW, are there any unintended side effects?
>>>> Perhaps a different issue but perhaps 64 bit offsets should be used
>>>> everywhere
>>>>
>>>> src/java.base/windows/native/libjava/io_util_md.c
>>>>   - Line 592: Using INVALID_HANDLE_VALUE is better than -1 and is used
>>>> elsewhere in the file
>>>> BTW, Testing for invalid handle might be unnecessary since the call
>>>> to GetFileSizeEx will fail
>>>> if it is invalid, yielding the same result.
>>>>
>>>> Roger
>>>>
>>>> On 12/10/2015 5:52 AM, vyom wrote:
>>>>> Hi All,
>>>>>
>>>>> Please review my changes for below bug.
>>>>>
>>>>> Bug: JDK-4823133 : RandomAccessFile.length() is not thread-safe
>>>>>
>>>>> Webrev:http://cr.openjdk.java.net/~vtewari/4823133/webrev0.0/
>>>>> <http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.0/>
>>>>>
>>>>> This change ensure that  length() does not temporarily changes the
>>>>> file pointer and it will make sure that there is no race
>>>>> condition in case of multi thread uses.
>>>>>
>>>>> Thanks,
>>>>> Vyom
>>>>>
>>>>>
>>>>>
>>>>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR 4823133: RandomAccessFile.length() is not thread-safe

[Dmitry Samersoff]

Vyom,

If I read the changes correctly, current code returns result of lseek()
but your code returns result of fstat().

I'm not sure it's a correct replacement.


dooku:test#truncate --size=102400 test.me

dooku:test#./test
STAT: 102400 0 Success
SEEK: 2 0 Success

Moreover, if you truncate a file to value that large than available free
space, lseek returns appropriate error but stat - not.

-Dmitry



On 2015-12-16 11:56, vyom wrote:
> Hi All,
> 
> Please find the updated
> webrev(http://cr.openjdk.java.net/~vtewari/4823133/webrev0.1/
> <http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.1/>). I
> incorporated the review comments by Roger Riggs.
> 
> Thanks,
> Vyom
> 
> 
> On Tuesday 15 December 2015 10:01 PM, Roger Riggs wrote:
>> Hi Yvom,
>>
>> Minor comments:
>>
>> src/java.base/share/native/libjava/RandomAccessFile.c:
>>  - "length fail" might be clearer as "GetLength failed"
>>
>> src/java.base/unix/native/libjava/io_util_md.c:
>>
>>  - Please add a comment before the define of FILE_OFFSET_BITS to
>> indicate where it is used and why it is there.
>>  - BTW, are there any unintended side effects?
>>Perhaps a different issue but perhaps 64 bit offsets should be used
>> everywhere
>>
>> src/java.base/windows/native/libjava/io_util_md.c
>>  - Line 592: Using INVALID_HANDLE_VALUE is better than -1 and is used
>> elsewhere in the file
>>BTW, Testing for invalid handle might be unnecessary since the call
>> to GetFileSizeEx will fail
>>if it is invalid, yielding the same result.
>>
>> Roger
>>
>> On 12/10/2015 5:52 AM, vyom wrote:
>>> Hi All,
>>>
>>> Please review my changes for below bug.
>>>
>>> Bug: JDK-4823133 : RandomAccessFile.length() is not thread-safe
>>>
>>> Webrev:http://cr.openjdk.java.net/~vtewari/4823133/webrev0.0/
>>> <http://cr.openjdk.java.net/%7Evtewari/4823133/webrev0.0/>
>>>
>>> This change ensure that  length() does not temporarily changes the
>>> file pointer and it will make sure that there is no race
>>> condition in case of multi thread uses.
>>>
>>> Thanks,
>>> Vyom
>>>
>>>
>>>
>>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: Code Review for JEP 259: Stack-Walking API

[Dmitry Samersoff]

Mandy,

Native part looks good for me.

javaClasses.cpp:

1. It might be good to create a helper inline function for

  int bci_version = stackFrame->int_field(bci_offset);
  int version = BackTrace::version_at(bci_version);

2. java_lang_StackFrameInfo::fill_methodInfo

  CHECK macro left methodInfo partially initialized, not sure it's OK.

javaClasses.inline.hpp:

78 You can use the same pattern as in assert:

if ((jushort)version != version) version = USHRT_MAX;

117 Is it possible to add comment about +100 magic?

jvm.cpp:

627 missed space around =

-Dmitry


On 2015-11-10 05:32, Mandy Chung wrote:
> javadoc:
>
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
> 
> webrev:
>   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
> 
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a 
> native frame (callStackWalk) that will fetch the first batch of stack frames. 
>  VM will then invoke the doStackWalk method so that the consumer starts 
> getting StackFrame object for each frame.  If all frames in the current batch 
> are traversed, it will ask the VM to fetch the next batch.  The library side 
> is doing the filtering of reflection frames.   For this patch, the VM filters 
> of the hidden frames and also filter out Throwable::init related frames for 
> stack trace.
> 
> Ultimately we will move to these built-in logic out from the VM to the 
> library but I’d like to separate them as future works.
> 
> This patch also includes the change for Throwable to use StackWalker but it’s 
> disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The 
> VM backtrace is well tuned for performance.  So we separate the switch of 
> Throwable to use StackWalker as a follow-on work:
>JDK-8141239 Throwable should use StackWalker API to capture the backtrace
> 
> MemberName initialization is one source of overhead and we propose to keep 
> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
> performance work to continue for JDK-8141239.  
> 
> Thanks
> Mandy
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: dlsym(RTLD_DEFAULT, "getentropy") return non-NULL on Mac

[Dmitry Samersoff]

Wang Weijun,

> The function is rather new in the latest Solaris beta [1] and it's
> preferred to reading from /dev/random. There are already people
> suggest adding it to Linux. If I use simply using dlsym then it will
> automatically work on all current and future platforms. In fact, I
> was planning to write

1. Please, check libc only not entire process image.

2. OS X random system is a different story: /dev/random never blocks and
returns the output of Yarrow PRNG.  Also OS X uses SecurityServer
process to feed entropy pool.

So IMHO, it's better to don't attempt to use other functions on OS X
until it appears officially.

3. Please notice that:

   /dev/random will block if you request more bits than entropy pool can
provide.

   but (according to manual page) getentropy will return immediately
with error if less that requested bytes of entropy is available and you
can't request more than 256 bytes of entropy at once.

it might require changes in uplevel logic.

-Dmitry


On 2015-11-08 03:37, Wang Weijun wrote:
> 
>> On Nov 8, 2015, at 4:29 AM, Dmitry Samersoff
>> <dmitry.samers...@oracle.com> wrote:
>> 
>> Wang Weijun,
>> 
>> 1. RTLD_DEFAUL call is expensive and dangerous because it cause
>> symbol search across all loaded images. So it can pick up something
>> absolutely irrelevant to your expectations at any time.
>> 
>> If you decide to play to this game, you have to use dladdr to check
>> the origin of a symbol and try to guess whether it is what you are
>> looking for or not.
>> 
>> Please dlopen(libSystem.dylib, RTLD_NOW) and search through it
>> only.
>> 
>> 2. You shouldn't rely on return value of dlopen() or dlsym(). Call 
>> dlerror() and check whether it returns error or NULL.
> 
> I'll try.
> 
>> 
>> PS: What is the goal of using a get entropy ?
> 
> The function is rather new in the latest Solaris beta [1] and it's
> preferred to reading from /dev/random. There are already people
> suggest adding it to Linux. If I use simply using dlsym then it will
> automatically work on all current and future platforms. In fact, I
> was planning to write
> 
> getentropy = (GETENTROPY_FN)dlsym(RTLD_DEFAULT, "getentropy"); if
> (getentropy) { return 1; } else { getrandom =
> (GETRANDOM_FN)dlsym(RTLD_DEFAULT, "getrandom"); if (getrandom) { 
> return 2; } else { return -1;  // will read /dev/random } }
> 
> Thanks Max
> 
> [1]
> https://blogs.oracle.com/darren/entry/solaris_new_system_calls_getentropy
>
> 
>> 
>> -Dmitry
>> 
>> On 2015-11-07 05:51, Wang Weijun wrote:
>>> I find something strange.
>>> 
>>> Background: a new method getentropy() is available on OpenBSD [1]
>>> and Solaris and people are also proposing it on other OSes.
>>> 
>>> Therefore inside JDK I write a piece of native code to detect it,
>>> something like
>>> 
>>> typedef int (*GETENTROPY_FN)(char* buffer, int len);
>>> 
>>> getentropy = (GETENTROPY_FN)dlsym(RTLD_DEFAULT, "getentropy"); if
>>> (getentropy) { return 1; }
>>> 
>>> On Mac, it returns non-NULL, but a later call to
>>> (*getentropy)(cbuffer, length) shows
>>> 
>>> #  SIGBUS (0xa) at pc=0x000103bfa030, pid=22434, tid=5891 
>>> ... # Problematic frame: # C  [libj2rand.dylib+0x1030]
>>> getentropy+0x0
>>> 
>>> However, "man getentropy" does not show anything, and the
>>> following simple program also prints out 0x0
>>> 
>>> #include  #include 
>>> 
>>> main() { void* g = dlsym(RTLD_DEFAULT, "getentropy"); 
>>> printf("%p\n", g); }
>>> 
>>> What does this mean? Is the JDK code loading another getentropy()
>>> somewhere else? How do I detect it and what shall I do to avoid
>>> it?
>>> 
>>> Thanks Max
>>> 
>>> [1]
>>> http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2
>>>
>>
>>
>>
>>> 
-- 
>> Dmitry Samersoff Oracle Java development team, Saint Petersburg,
>> Russia * I would love to change the world, but they won't give me
>> the sources.
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: dlsym(RTLD_DEFAULT, "getentropy") return non-NULL on Mac

[Dmitry Samersoff]

Wang Weijun,

1. RTLD_DEFAUL call is expensive and dangerous because it cause symbol
search across all loaded images. So it can pick up something absolutely
irrelevant to your expectations at any time.

If you decide to play to this game, you have to use dladdr to check the
origin of a symbol and try to guess whether it is what you are looking
for or not.

Please dlopen(libSystem.dylib, RTLD_NOW) and search through it only.

2. You shouldn't rely on return value of dlopen() or dlsym(). Call
dlerror() and check whether it returns error or NULL.

PS:
  What is the goal of using a getentropy ?

-Dmitry

On 2015-11-07 05:51, Wang Weijun wrote:
> I find something strange.
> 
> Background: a new method getentropy() is available on OpenBSD [1] and Solaris 
> and people are also proposing it on other OSes.
> 
> Therefore inside JDK I write a piece of native code to detect it, something 
> like
> 
> typedef int (*GETENTROPY_FN)(char* buffer, int len);
> 
> getentropy = (GETENTROPY_FN)dlsym(RTLD_DEFAULT, "getentropy");
> if (getentropy) {
> return 1;
> } 
> 
> On Mac, it returns non-NULL, but a later call to (*getentropy)(cbuffer, 
> length) shows
> 
>   #  SIGBUS (0xa) at pc=0x000103bfa030, pid=22434, tid=5891
>   ...
>   # Problematic frame:
>   # C  [libj2rand.dylib+0x1030]  getentropy+0x0
> 
> However, "man getentropy" does not show anything, and the following simple 
> program also prints out 0x0
> 
> #include 
> #include 
> 
> main() {
>void* g = dlsym(RTLD_DEFAULT, "getentropy");
>printf("%p\n", g);
> }
> 
> What does this mean? Is the JDK code loading another getentropy() somewhere 
> else? How do I detect it and what shall I do to avoid it?
> 
> Thanks
> Max
> 
> [1] http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Staffan,

Could you review a CCC request.

http://ccc.us.oracle.com/8059036

-Dmitry

On 2015-06-05 15:24, Staffan Larsen wrote:
 Thanks - this version looks good to me.
 
 /Staffan
 
 On 5 jun 2015, at 13:00, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Staffan,

 Thank you for review!

 Done. Webrev updated in-place (press shift-reload).

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

 -Dmitry

 On 2015-06-05 11:20, Staffan Larsen wrote:
 Dmitry,

 I’d like to propose the following change to get prettier output (more in 
 line with GC.class_histogram):

 diff --git a/src/share/vm/services/diagnosticCommand.cpp 
 b/src/share/vm/services/diagnosticCommand.cpp
 --- a/src/share/vm/services/diagnosticCommand.cpp
 +++ b/src/share/vm/services/diagnosticCommand.cpp
 @@ -341,7 +341,6 @@
 void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
   ResourceMark rm;

 -  output()-print_cr(Unreachable instances awaiting finalization:);
   Klass* k = SystemDictionary::resolve_or_null(
 vmSymbols::finalizer_histogram_klass(), THREAD);
   assert(k != NULL, FinalizerHistogram class is not accessible);
 @@ -375,12 +374,15 @@

   assert(count_res != NULL  name_res != NULL, Unexpected layout of 
 FinalizerHistogramEntry);

 +  output()-print_cr(Unreachable instances waiting for finalization);
 +  output()-print_cr(#instances  class name);
 +  output()-print_cr(---);
   for (int i = 0; i  result_oop-length(); ++i) {
 oop element_oop = result_oop-obj_at(i);
 oop str_oop = element_oop-obj_field(name_fd.offset());
 char *name = java_lang_String::as_utf8_string(str_oop);
 int count = element_oop-int_field(count_fd.offset());
 -output()-print_cr(Class %s - %d, name, count);
 +output()-print_cr(%10d  %s, count, name);
   }
 }


 A couple of nits:

 diagnosticCommand.cpp:359 - extra space after =
 diagnosticCommand.cpp:361 - spelling: finlalization - finalization
 FinalizerInfoTest.java:69: - spelling: intialized - initialized
 FinalizerInfoTest.java:71 - I’d like to see the ; on a new line


 Thanks,
 /Staffan


 On 5 jun 2015, at 00:20, Mandy Chung mandy.ch...@oracle.com wrote:


 On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff 
 dmitry.samers...@oracle.com wrote:

 Mandy,

 Webrev updated in-place (press shift-reload).

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

 getFinalizerHistogram() now returns Entry[]

 @library and @build removed from the test.


 Looks fine.

 Thanks
 Mandy


 -Dmitry

 On 2015-06-04 16:56, Mandy Chung wrote:

 On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff 
 dmitry.samers...@oracle.com wrote:

 Mandy,

 Thank you for the review.

 Updated webrev is:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14


 Thanks for the update and the test.

 addressed comments, added a unit test to JDK workspace.


 This test does not need @library and @build, right?  


 On 2015-06-03 21:36, Mandy Chung wrote:

 Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
 The VM knows the returned type anyway.

 [java/lang/ref/Entry signature would need a separate symbol entry in
 swollen vmSymbols::init() so I would prefer to stay with more generic
 Object[]


 You already add several symbols - why is it an issue for another one?  
 Or if adding vm symbols is a concern, you should revert to String and 
 int[] that you decide not to.   The decision should apply consistently 
 (use String and int, or new Java type).


 Perhaps the getFinalizerHistogram method should be renamed
 e.g. dump?

 The line in vmSymbols looks like:

 template(
 get_finalizer_histogram_name, getFinalizerHistogram)

 I would prefer to keep method name specific enough to be able to
 find it by grep in jdk code.


 Grep in jdk code is easy as you have a new class :)  

 Mandy


 (other comments are addressed)

 -Dmitry


 On 2015-06-03 21:36, Mandy Chung wrote:


 On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
 Updated webrev:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13

 I reviewed the jdk change.  The version looks okay and some comments:

 ReferenceQueue.java - you should eliminate the use of rawtype:

 84 Reference rn = r.next;

 Change Reference to Reference? extends T

 done.


 The forEach method - eliminate the use of raw type and
 move @SuppressWarning to the field variable in line 182:

   @SuppressWarnings(unchecked)
   Reference? extends T rn = r.next;

 done.


 FinalizerHistogram.java
 Copyright year needs update.

 done.

 I personally think the VM code would be much simpler if you stay with
 alternative entry of String and int than dealing with a
 FinalizerHistogramEntry private class.  It's okay as FinalizerHistogram
 class is separated.

 The comment in line 35 is suitable to move to the class description and
 this is the suggestion.

 // This FinalizerHistogram class is for GC.finalizer_info diagnostic
 command support.
 // It is invoked by the VM.

 done.


 Should getFinalizerHistogram

Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Staffan,

Thank you for review!

Done. Webrev updated in-place (press shift-reload).

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

-Dmitry

On 2015-06-05 11:20, Staffan Larsen wrote:
 Dmitry,
 
 I’d like to propose the following change to get prettier output (more in line 
 with GC.class_histogram):
 
 diff --git a/src/share/vm/services/diagnosticCommand.cpp 
 b/src/share/vm/services/diagnosticCommand.cpp
 --- a/src/share/vm/services/diagnosticCommand.cpp
 +++ b/src/share/vm/services/diagnosticCommand.cpp
 @@ -341,7 +341,6 @@
  void FinalizerInfoDCmd::execute(DCmdSource source, TRAPS) {
ResourceMark rm;
 
 -  output()-print_cr(Unreachable instances awaiting finalization:);
Klass* k = SystemDictionary::resolve_or_null(
  vmSymbols::finalizer_histogram_klass(), THREAD);
assert(k != NULL, FinalizerHistogram class is not accessible);
 @@ -375,12 +374,15 @@
 
assert(count_res != NULL  name_res != NULL, Unexpected layout of 
 FinalizerHistogramEntry);
 
 +  output()-print_cr(Unreachable instances waiting for finalization);
 +  output()-print_cr(#instances  class name);
 +  output()-print_cr(---);
for (int i = 0; i  result_oop-length(); ++i) {
  oop element_oop = result_oop-obj_at(i);
  oop str_oop = element_oop-obj_field(name_fd.offset());
  char *name = java_lang_String::as_utf8_string(str_oop);
  int count = element_oop-int_field(count_fd.offset());
 -output()-print_cr(Class %s - %d, name, count);
 +output()-print_cr(%10d  %s, count, name);
}
  }
 
 
 A couple of nits:
 
 diagnosticCommand.cpp:359 - extra space after =
 diagnosticCommand.cpp:361 - spelling: finlalization - finalization
 FinalizerInfoTest.java:69: - spelling: intialized - initialized
 FinalizerInfoTest.java:71 - I’d like to see the ; on a new line
 
 
 Thanks,
 /Staffan
 
 
 On 5 jun 2015, at 00:20, Mandy Chung mandy.ch...@oracle.com wrote:


 On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Mandy,

 Webrev updated in-place (press shift-reload).

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

 getFinalizerHistogram() now returns Entry[]

 @library and @build removed from the test.


 Looks fine.

 Thanks
 Mandy


 -Dmitry

 On 2015-06-04 16:56, Mandy Chung wrote:

 On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff 
 dmitry.samers...@oracle.com wrote:

 Mandy,

 Thank you for the review.

 Updated webrev is:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14


 Thanks for the update and the test.

 addressed comments, added a unit test to JDK workspace.


 This test does not need @library and @build, right?  


 On 2015-06-03 21:36, Mandy Chung wrote:

 Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
 The VM knows the returned type anyway.

 [java/lang/ref/Entry signature would need a separate symbol entry in
 swollen vmSymbols::init() so I would prefer to stay with more generic
 Object[]


 You already add several symbols - why is it an issue for another one?  Or 
 if adding vm symbols is a concern, you should revert to String and int[] 
 that you decide not to.   The decision should apply consistently (use 
 String and int, or new Java type).


 Perhaps the getFinalizerHistogram method should be renamed
 e.g. dump?

 The line in vmSymbols looks like:

 template(
 get_finalizer_histogram_name, getFinalizerHistogram)

 I would prefer to keep method name specific enough to be able to
 find it by grep in jdk code.


 Grep in jdk code is easy as you have a new class :)  

 Mandy


 (other comments are addressed)

 -Dmitry


 On 2015-06-03 21:36, Mandy Chung wrote:


 On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
 Updated webrev:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13

 I reviewed the jdk change.  The version looks okay and some comments:

 ReferenceQueue.java - you should eliminate the use of rawtype:

 84 Reference rn = r.next;

 Change Reference to Reference? extends T

 done.


 The forEach method - eliminate the use of raw type and
 move @SuppressWarning to the field variable in line 182:

@SuppressWarnings(unchecked)
Reference? extends T rn = r.next;

 done.


 FinalizerHistogram.java
 Copyright year needs update.

 done.

 I personally think the VM code would be much simpler if you stay with
 alternative entry of String and int than dealing with a
 FinalizerHistogramEntry private class.  It's okay as FinalizerHistogram
 class is separated.

 The comment in line 35 is suitable to move to the class description and
 this is the suggestion.

  // This FinalizerHistogram class is for GC.finalizer_info diagnostic
 command support.
  // It is invoked by the VM.

 done.


 Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
 knows the returned type anyway.  

 [java/lang/ref/Entry signature would need a separate symbol entry in
 swollen vmSymbols::init() so I would prefer to stay with more generic
 Object[]

 It's an inner

Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Mandy,

Webrev updated in-place (press shift-reload).

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

getFinalizerHistogram() now returns Entry[]

@library and @build removed from the test.

-Dmitry

On 2015-06-04 16:56, Mandy Chung wrote:
 
 On Jun 4, 2015, at 6:38 AM, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Mandy,

 Thank you for the review.

 Updated webrev is:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

 
 Thanks for the update and the test.
 
 addressed comments, added a unit test to JDK workspace.

 
 This test does not need @library and @build, right?  
 

 On 2015-06-03 21:36, Mandy Chung wrote:

 Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
 The VM knows the returned type anyway.

 [java/lang/ref/Entry signature would need a separate symbol entry in
 swollen vmSymbols::init() so I would prefer to stay with more generic
 Object[]

 
 You already add several symbols - why is it an issue for another one?  Or if 
 adding vm symbols is a concern, you should revert to String and int[] that 
 you decide not to.   The decision should apply consistently (use String and 
 int, or new Java type).
 
 
 Perhaps the getFinalizerHistogram method should be renamed
 e.g. dump?

 The line in vmSymbols looks like:

 template(
 get_finalizer_histogram_name, getFinalizerHistogram)

 I would prefer to keep method name specific enough to be able to
 find it by grep in jdk code.
 
 
 Grep in jdk code is easy as you have a new class :)  
 
 Mandy
 

 (other comments are addressed)

 -Dmitry


 On 2015-06-03 21:36, Mandy Chung wrote:


 On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
 Updated webrev:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13

 I reviewed the jdk change.  The version looks okay and some comments:

 ReferenceQueue.java - you should eliminate the use of rawtype:

 84 Reference rn = r.next;

 Change Reference to Reference? extends T

 done.


 The forEach method - eliminate the use of raw type and
 move @SuppressWarning to the field variable in line 182:

  @SuppressWarnings(unchecked)
  Reference? extends T rn = r.next;

 done.


 FinalizerHistogram.java
  Copyright year needs update.

 done.

 I personally think the VM code would be much simpler if you stay with
 alternative entry of String and int than dealing with a
 FinalizerHistogramEntry private class.  It's okay as FinalizerHistogram
 class is separated.

 The comment in line 35 is suitable to move to the class description and
 this is the suggestion.

// This FinalizerHistogram class is for GC.finalizer_info diagnostic
 command support.
// It is invoked by the VM.

 done.


 Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
 knows the returned type anyway.  

 [java/lang/ref/Entry signature would need a separate symbol entry in
 swollen vmSymbols::init() so I would prefer to stay with more generic
 Object[]

 It's an inner class of
 FinalizerHistogram and it can simply be named as Entry.   I suggest to
 have Entry::increment method to replace .instanceCount++.

 done.


 The tests are in hotspot/test.Can you add a unit test in jdk/test? 
 Perhaps you can test FinalizerHistogram.getFinalizerHistogram() via
 reflection - just a sanity test.

 done. The test repeats hotspot one, but uses reflection instead of VM call.


 A minor naming comment - now you have a FinalizerHistogram class.
 Perhaps the getFinalizerHistogram method should be renamed e.g. dump?

 The line in vmSymbols looks like:

 template(
 get_finalizer_histogram_name, getFinalizerHistogram)

 I would prefer to keep it specific enough to be able to
 find it by grep in jdk code.


 -- 
 Dmitry Samersoff
 Oracle Java development team, Saint Petersburg, Russia
 * I would love to change the world, but they won't give me the sources.
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M, v14): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Mandy,

Thank you for the review.

Updated webrev is:

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.14

addressed comments, added a unit test to JDK workspace.


On 2015-06-03 21:36, Mandy Chung wrote:

 Should getFinalizerHistogram() return FinalizerHistogramEntry[]?
 The VM knows the returned type anyway.

[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]

 Perhaps the getFinalizerHistogram method should be renamed
 e.g. dump?

The line in vmSymbols looks like:

template(
get_finalizer_histogram_name, getFinalizerHistogram)

I would prefer to keep method name specific enough to be able to
find it by grep in jdk code.

(other comments are addressed)

-Dmitry


On 2015-06-03 21:36, Mandy Chung wrote:
 
 
 On 06/03/2015 08:29 AM, Dmitry Samersoff wrote:
 Updated webrev:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13
 
 I reviewed the jdk change.  The version looks okay and some comments:
 
 ReferenceQueue.java - you should eliminate the use of rawtype:
 
 84 Reference rn = r.next;
 
 Change Reference to Reference? extends T

done.

 
 The forEach method - eliminate the use of raw type and
 move @SuppressWarning to the field variable in line 182:
 
   @SuppressWarnings(unchecked)
   Reference? extends T rn = r.next;

done.

 
 FinalizerHistogram.java
   Copyright year needs update.

done.
 
 I personally think the VM code would be much simpler if you stay with
 alternative entry of String and int than dealing with a
 FinalizerHistogramEntry private class.  It's okay as FinalizerHistogram
 class is separated.
 
 The comment in line 35 is suitable to move to the class description and
 this is the suggestion.
 
 // This FinalizerHistogram class is for GC.finalizer_info diagnostic
 command support.
 // It is invoked by the VM.

done.

 
 Should getFinalizerHistogram() return FinalizerHistogramEntry[]? The VM
 knows the returned type anyway.  

[java/lang/ref/Entry signature would need a separate symbol entry in
swollen vmSymbols::init() so I would prefer to stay with more generic
Object[]

 It's an inner class of
 FinalizerHistogram and it can simply be named as Entry.   I suggest to
 have Entry::increment method to replace .instanceCount++.

done.

 
 The tests are in hotspot/test.Can you add a unit test in jdk/test? 
 Perhaps you can test FinalizerHistogram.getFinalizerHistogram() via
 reflection - just a sanity test.

done. The test repeats hotspot one, but uses reflection instead of VM call.

 
 A minor naming comment - now you have a FinalizerHistogram class.
 Perhaps the getFinalizerHistogram method should be renamed e.g. dump?

The line in vmSymbols looks like:

template(
get_finalizer_histogram_name, getFinalizerHistogram)

I would prefer to keep it specific enough to be able to
find it by grep in jdk code.


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M, v12): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Everyone,

Updated webrev:

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.12

getFinalizerHistogram and FinalizerHistogramEntry moved to separate
package-private class. Hotspot code changed accordingly.

getFinalizerHistogram updated to use Peter's code.

-Dmitry


On 2015-06-03 09:06, Peter Levart wrote:
 Hi Dmitry,
 
 On 06/02/2015 01:12 PM, Dmitry Samersoff wrote:
 Staffan,

 Instead of hardcoding the field offsets, you can use
 InstanceKlass::find_field and fieldDescriptor::offset to find the
 offset at runtime.
 Done. Please, see

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11
 
 In the jdk part, now that you have a FinalizerHistogramEntry class, you
 can simplify the code even further:
 
 
 private static final class FinalizerHistogramEntry {
 int instanceCount;
 final String className;
 
 int getInstanceCount() {
 return instanceCount;
 }
 
 FinalizerHistogramEntry(String className) {
 this.className = className;
 }
 }
 
 static Object[] getFinalizerHistogram() {
 MapString, FinalizerHistogramEntry countMap = new HashMap();
 queue.forEach(r - {
 Object referent = r.get();
 if (referent != null) {
 countMap.computeIfAbsent(
 referent.getClass().getName(),
 FinalizerHistogramEntry::new).instanceCount++;
 /* Clear stack slot containing this variable, to decrease
the chances of false retention with a conservative GC */
 referent = null;
 }
 });
 
 FinalizerHistogramEntry fhe[] = countMap.values()
 .toArray(new FinalizerHistogramEntry[countMap.size()]);
 Arrays.sort(fhe,
 Comparator.comparingInt(

 FinalizerHistogramEntry::getInstanceCount).reversed());
 return fhe;
 }
 
 
 ... see, no copying loop required. Also notice the access modifier used
 on the nested class and it's fields/method/constructor. This way the
 class is private and fields can be accessed from getFinalizerHistogram
 and lambda without compiler generating access bridges.
 
 
 Regards, Peter
 

 I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
 oop.inline.hpp leaving a room for further cleanup because I found couple
 of places in hotspot that implements mostly similar functionality.

 -Dmitry

 On 2015-06-01 10:18, Staffan Larsen wrote:
 Dmitry,

 Instead of hardcoding the field offsets, you can use 
 InstanceKlass::find_field and fieldDescriptor::offset to find the offset at 
 runtime. 

 Thanks,
 /Staffan

 On 31 maj 2015, at 13:43, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Everyone,

 Please take a look at new version of the fix.

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

 Changes (to previous version) are in
 Finalizer.java and diagnosticCommand.cpp

 This version copy data from Map.Entry to array of
 FinalizerHistogramEntry instances then,
 VM prints content of this array.

 -Dmitry


 On 2015-05-28 21:06, Mandy Chung wrote:
 On 05/28/2015 07:35 AM, Peter Levart wrote:
 Hi Mandy,

 On 05/27/2015 03:32 PM, Mandy Chung wrote:
 Taking it further - is it simpler to return String[] of all
 classnames including the duplicated ones and have the VM do the
 count?  Are you concerned with the size of the String[]?
 Yes, the histogram is much smaller than the list of all instances.
 There can be millions of instances waiting in finalizer queue, but
 only a few distinct classes.
 Do you happen to know what libraries are the major contributors to these
 millions of finalizers?

 It has been strongly recommended to avoid finalizers (see Effective Java
 Item 7).   I'm not surprised that existing code is still using
 finalizers while we should really encourage them to migrate away from it.

 What could be done in Java to simplify things in native code but still
 not format the output is to convert the array of Map.Entry(s) into an
 Object[] array of alternating {String, int[], String, int[],  }

 Would this simplify native code for the price of a little extra work
 in Java? The sizes of old and new arrays are not big (# of distinct
 classes of finalizable objects in the queue).
 I also prefer writing in Java and writing less native code (much
 simplified).  It's an interface that we have to agree upon and keep it
 simple.  Having the returned Object[] as alternate String and int[] is a
 reasonable compromise.

 ReferenceQueue.java - you can move @SuppressWarning from the method to
 just the field variable rn
 @SuppressWarnings(unchecked)
 Reference? extends T rn = r.next;

 Finalizer.java
 It's better to rename printFinalizationQueue as it inspects the
 finalizer reference queue (maybe getFinalizerHistogram?).  Can you move
 this method to the end of this class and add the comment saying that
 this is invoked

Re: RFR(M, v13): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Everyone,

Updated webrev:

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.13

Changes to oop.inline.hpp is reverted and find_field used directly is
diagnostic command.

-Dmitry

On 2015-06-03 11:48, Dmitry Samersoff wrote:
 Everyone,
 
 Updated webrev:
 
 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.12
 
 getFinalizerHistogram and FinalizerHistogramEntry moved to separate
 package-private class. Hotspot code changed accordingly.
 
 getFinalizerHistogram updated to use Peter's code.
 
 -Dmitry
 
 
 On 2015-06-03 09:06, Peter Levart wrote:
 Hi Dmitry,

 On 06/02/2015 01:12 PM, Dmitry Samersoff wrote:
 Staffan,

 Instead of hardcoding the field offsets, you can use
 InstanceKlass::find_field and fieldDescriptor::offset to find the
 offset at runtime.
 Done. Please, see

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11

 In the jdk part, now that you have a FinalizerHistogramEntry class, you
 can simplify the code even further:


 private static final class FinalizerHistogramEntry {
 int instanceCount;
 final String className;

 int getInstanceCount() {
 return instanceCount;
 }

 FinalizerHistogramEntry(String className) {
 this.className = className;
 }
 }

 static Object[] getFinalizerHistogram() {
 MapString, FinalizerHistogramEntry countMap = new HashMap();
 queue.forEach(r - {
 Object referent = r.get();
 if (referent != null) {
 countMap.computeIfAbsent(
 referent.getClass().getName(),
 FinalizerHistogramEntry::new).instanceCount++;
 /* Clear stack slot containing this variable, to decrease
the chances of false retention with a conservative GC */
 referent = null;
 }
 });

 FinalizerHistogramEntry fhe[] = countMap.values()
 .toArray(new FinalizerHistogramEntry[countMap.size()]);
 Arrays.sort(fhe,
 Comparator.comparingInt(

 FinalizerHistogramEntry::getInstanceCount).reversed());
 return fhe;
 }


 ... see, no copying loop required. Also notice the access modifier used
 on the nested class and it's fields/method/constructor. This way the
 class is private and fields can be accessed from getFinalizerHistogram
 and lambda without compiler generating access bridges.


 Regards, Peter


 I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
 oop.inline.hpp leaving a room for further cleanup because I found couple
 of places in hotspot that implements mostly similar functionality.

 -Dmitry

 On 2015-06-01 10:18, Staffan Larsen wrote:
 Dmitry,

 Instead of hardcoding the field offsets, you can use 
 InstanceKlass::find_field and fieldDescriptor::offset to find the offset 
 at runtime. 

 Thanks,
 /Staffan

 On 31 maj 2015, at 13:43, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Everyone,

 Please take a look at new version of the fix.

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

 Changes (to previous version) are in
 Finalizer.java and diagnosticCommand.cpp

 This version copy data from Map.Entry to array of
 FinalizerHistogramEntry instances then,
 VM prints content of this array.

 -Dmitry


 On 2015-05-28 21:06, Mandy Chung wrote:
 On 05/28/2015 07:35 AM, Peter Levart wrote:
 Hi Mandy,

 On 05/27/2015 03:32 PM, Mandy Chung wrote:
 Taking it further - is it simpler to return String[] of all
 classnames including the duplicated ones and have the VM do the
 count?  Are you concerned with the size of the String[]?
 Yes, the histogram is much smaller than the list of all instances.
 There can be millions of instances waiting in finalizer queue, but
 only a few distinct classes.
 Do you happen to know what libraries are the major contributors to these
 millions of finalizers?

 It has been strongly recommended to avoid finalizers (see Effective Java
 Item 7).   I'm not surprised that existing code is still using
 finalizers while we should really encourage them to migrate away from it.

 What could be done in Java to simplify things in native code but still
 not format the output is to convert the array of Map.Entry(s) into an
 Object[] array of alternating {String, int[], String, int[],  }

 Would this simplify native code for the price of a little extra work
 in Java? The sizes of old and new arrays are not big (# of distinct
 classes of finalizable objects in the queue).
 I also prefer writing in Java and writing less native code (much
 simplified).  It's an interface that we have to agree upon and keep it
 simple.  Having the returned Object[] as alternate String and int[] is a
 reasonable compromise.

 ReferenceQueue.java - you can move @SuppressWarning from the method to
 just the field variable rn
 @SuppressWarnings(unchecked)
 Reference? extends T rn = r.next

Re: RFR(M, v11): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Mikael,

The reason of placing get_filed_offset_by_name to the oop.inline is that
hotspot widely duplicate this code.

Symbol is used to identify a field within klass, not a klass them self
so I think it's OK to have it tied to the oopDesc.

-Dmitry

On 2015-06-02 15:06, Mikael Gerdin wrote:
 Hi Dmitry,
 
 On 2015-06-02 13:12, Dmitry Samersoff wrote:
 Staffan,

 Instead of hardcoding the field offsets, you can use
 InstanceKlass::find_field and fieldDescriptor::offset to find the
 offset at runtime.

 Done. Please, see

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11

 I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
 oop.inline.hpp leaving a room for further cleanup because I found couple
 of places in hotspot that implements mostly similar functionality.
 
 Sorry for this sort of drive-by review, but I really don't think
 get_field_offset_by_name should be in the oop class. If anywhere it
 should be on InstanceKlass, and using Symbol* to identify a Klass* seems
 incorrect since the same symbol can refer to different classes in
 different class loader contexts.
 
 /Mikael
 

 -Dmitry

 On 2015-06-01 10:18, Staffan Larsen wrote:
 Dmitry,

 Instead of hardcoding the field offsets, you can use
 InstanceKlass::find_field and fieldDescriptor::offset to find the
 offset at runtime.

 Thanks,
 /Staffan

 On 31 maj 2015, at 13:43, Dmitry Samersoff
 dmitry.samers...@oracle.com wrote:

 Everyone,

 Please take a look at new version of the fix.

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

 Changes (to previous version) are in
 Finalizer.java and diagnosticCommand.cpp

 This version copy data from Map.Entry to array of
 FinalizerHistogramEntry instances then,
 VM prints content of this array.

 -Dmitry


 On 2015-05-28 21:06, Mandy Chung wrote:

 On 05/28/2015 07:35 AM, Peter Levart wrote:
 Hi Mandy,

 On 05/27/2015 03:32 PM, Mandy Chung wrote:
 Taking it further - is it simpler to return String[] of all
 classnames including the duplicated ones and have the VM do the
 count?  Are you concerned with the size of the String[]?

 Yes, the histogram is much smaller than the list of all instances.
 There can be millions of instances waiting in finalizer queue, but
 only a few distinct classes.

 Do you happen to know what libraries are the major contributors to
 these
 millions of finalizers?

 It has been strongly recommended to avoid finalizers (see Effective
 Java
 Item 7).   I'm not surprised that existing code is still using
 finalizers while we should really encourage them to migrate away
 from it.

 What could be done in Java to simplify things in native code but
 still
 not format the output is to convert the array of Map.Entry(s) into an
 Object[] array of alternating {String, int[], String, int[],  }

 Would this simplify native code for the price of a little extra work
 in Java? The sizes of old and new arrays are not big (# of distinct
 classes of finalizable objects in the queue).

 I also prefer writing in Java and writing less native code (much
 simplified).  It's an interface that we have to agree upon and keep it
 simple.  Having the returned Object[] as alternate String and int[]
 is a
 reasonable compromise.

 ReferenceQueue.java - you can move @SuppressWarning from the method to
 just the field variable rn
  @SuppressWarnings(unchecked)
  Reference? extends T rn = r.next;

 Finalizer.java
  It's better to rename printFinalizationQueue as it inspects the
 finalizer reference queue (maybe getFinalizerHistogram?).  Can you
 move
 this method to the end of this class and add the comment saying that
 this is invoked by VM for jcmd -finalizerinfo support and @return to
 describe the returned content.  I also think you can remove
 @SuppressWarnings for this method.

 Mandy


 -- 
 Dmitry Samersoff
 Oracle Java development team, Saint Petersburg, Russia
 * I would love to change the world, but they won't give me the sources.





-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M, v11): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Staffan,

 Instead of hardcoding the field offsets, you can use
 InstanceKlass::find_field and fieldDescriptor::offset to find the
 offset at runtime.

Done. Please, see

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.11

I put the function int get_filed_offset_by_name(Symbol*,Symbol*) to
oop.inline.hpp leaving a room for further cleanup because I found couple
of places in hotspot that implements mostly similar functionality.

-Dmitry

On 2015-06-01 10:18, Staffan Larsen wrote:
 Dmitry,
 
 Instead of hardcoding the field offsets, you can use 
 InstanceKlass::find_field and fieldDescriptor::offset to find the offset at 
 runtime. 
 
 Thanks,
 /Staffan
 
 On 31 maj 2015, at 13:43, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Everyone,

 Please take a look at new version of the fix.

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

 Changes (to previous version) are in
 Finalizer.java and diagnosticCommand.cpp

 This version copy data from Map.Entry to array of
 FinalizerHistogramEntry instances then,
 VM prints content of this array.

 -Dmitry


 On 2015-05-28 21:06, Mandy Chung wrote:

 On 05/28/2015 07:35 AM, Peter Levart wrote:
 Hi Mandy,

 On 05/27/2015 03:32 PM, Mandy Chung wrote:
 Taking it further - is it simpler to return String[] of all
 classnames including the duplicated ones and have the VM do the
 count?  Are you concerned with the size of the String[]?

 Yes, the histogram is much smaller than the list of all instances.
 There can be millions of instances waiting in finalizer queue, but
 only a few distinct classes.

 Do you happen to know what libraries are the major contributors to these
 millions of finalizers?

 It has been strongly recommended to avoid finalizers (see Effective Java
 Item 7).   I'm not surprised that existing code is still using
 finalizers while we should really encourage them to migrate away from it.

 What could be done in Java to simplify things in native code but still
 not format the output is to convert the array of Map.Entry(s) into an
 Object[] array of alternating {String, int[], String, int[],  }

 Would this simplify native code for the price of a little extra work
 in Java? The sizes of old and new arrays are not big (# of distinct
 classes of finalizable objects in the queue).

 I also prefer writing in Java and writing less native code (much
 simplified).  It's an interface that we have to agree upon and keep it
 simple.  Having the returned Object[] as alternate String and int[] is a
 reasonable compromise.

 ReferenceQueue.java - you can move @SuppressWarning from the method to
 just the field variable rn
 @SuppressWarnings(unchecked)
 Reference? extends T rn = r.next;

 Finalizer.java
 It's better to rename printFinalizationQueue as it inspects the
 finalizer reference queue (maybe getFinalizerHistogram?).  Can you move
 this method to the end of this class and add the comment saying that
 this is invoked by VM for jcmd -finalizerinfo support and @return to
 describe the returned content.  I also think you can remove
 @SuppressWarnings for this method.

 Mandy


 -- 
 Dmitry Samersoff
 Oracle Java development team, Saint Petersburg, Russia
 * I would love to change the world, but they won't give me the sources.
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M, v10): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Everyone,

Please take a look at new version of the fix.

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.10/

Changes (to previous version) are in
 Finalizer.java and diagnosticCommand.cpp

This version copy data from Map.Entry to array of
FinalizerHistogramEntry instances then,
VM prints content of this array.

-Dmitry


On 2015-05-28 21:06, Mandy Chung wrote:
 
 On 05/28/2015 07:35 AM, Peter Levart wrote:
 Hi Mandy,

 On 05/27/2015 03:32 PM, Mandy Chung wrote:
 Taking it further - is it simpler to return String[] of all
 classnames including the duplicated ones and have the VM do the
 count?  Are you concerned with the size of the String[]?

 Yes, the histogram is much smaller than the list of all instances.
 There can be millions of instances waiting in finalizer queue, but
 only a few distinct classes.
 
 Do you happen to know what libraries are the major contributors to these
 millions of finalizers?
 
 It has been strongly recommended to avoid finalizers (see Effective Java
 Item 7).   I'm not surprised that existing code is still using
 finalizers while we should really encourage them to migrate away from it.
 
 What could be done in Java to simplify things in native code but still
 not format the output is to convert the array of Map.Entry(s) into an
 Object[] array of alternating {String, int[], String, int[],  }

 Would this simplify native code for the price of a little extra work
 in Java? The sizes of old and new arrays are not big (# of distinct
 classes of finalizable objects in the queue).
 
 I also prefer writing in Java and writing less native code (much
 simplified).  It's an interface that we have to agree upon and keep it
 simple.  Having the returned Object[] as alternate String and int[] is a
 reasonable compromise.
 
 ReferenceQueue.java - you can move @SuppressWarning from the method to
 just the field variable rn
  @SuppressWarnings(unchecked)
  Reference? extends T rn = r.next;
 
 Finalizer.java
  It's better to rename printFinalizationQueue as it inspects the
 finalizer reference queue (maybe getFinalizerHistogram?).  Can you move
 this method to the end of this class and add the comment saying that
 this is invoked by VM for jcmd -finalizerinfo support and @return to
 describe the returned content.  I also think you can remove
 @SuppressWarnings for this method.
 
 Mandy


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M,v9): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Hi Everybody,

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.09/

Please review updated webrev -

printFinalizationQueue now returns and array of Map.EntryString, int[])
and all formatting is done on VM side.

-Dmitry

On 2015-05-21 02:07, Mandy Chung wrote:
 
 On May 19, 2015, at 11:51 PM, Dmitry Samersoff
 dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote:

 Other alternatives could be to do all hashing/sorting/printing on native
 layer i.e. implement printFinalizationQueue inside VM.

 Both options has pros and cons - Java based solution requires less JNI
 calls and better readable but takes more memory.

 It might be better to return an array of Map.EntryString, int[]
 objects to VM rather than one huge string.
 
 The output and formatting should be done by jcmd.  What you really need
 to get a peek on the finalizer queue and print the histogram.   The VM
 has the heap histogram implementation.  Have you considered leveraging
 that? 
 
5:  1012  40480  java.lang.ref.Finalizer
 
 You can find the registered Finalizer instances.  The downside is that
 icmd -finalizerinfo stops the world.  I think it’s not unreasonable for
 this diagnostic command to be expensive like -heap command.
 
 Mandy


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

On 2015-05-21 02:07, Mandy Chung wrote:
 
 On May 19, 2015, at 11:51 PM, Dmitry Samersoff
 dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote:

 Other alternatives could be to do all hashing/sorting/printing on native
 layer i.e. implement printFinalizationQueue inside VM.

 Both options has pros and cons - Java based solution requires less JNI
 calls and better readable but takes more memory.

 It might be better to return an array of Map.EntryString, int[]
 objects to VM rather than one huge string.
 
 The output and formatting should be done by jcmd.  

done.

 What you really need
 to get a peek on the finalizer queue and print the histogram.   The VM
 has the heap histogram implementation.  Have you considered leveraging
 that? 
 
5:  1012  40480  java.lang.ref.Finalizer

One of previous versions count total a number of instances registered
for finalization but then we decided that number of unreachable
instances awaiting finalization has more value for customer.

 You can find the registered Finalizer instances.  The downside is that
 icmd -finalizerinfo stops the world.  I think it’s not unreasonable for
 this diagnostic command to be expensive like -heap command.

Current implementation is lock-free and don't stop the world, we decided
to make it less expensive at the cost of less accurate results.

-Dmitry

-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Mandy,

 However I have trouble for
 Finalizer.printFinalizationQueue method that doesn’t belong there.
 What are the other alternatives you have explored?

Other alternatives could be to do all hashing/sorting/printing on native
layer i.e. implement printFinalizationQueue inside VM.

Both options has pros and cons - Java based solution requires less JNI
calls and better readable but takes more memory.

It might be better to return an array of Map.EntryString, int[]
objects to VM rather than one huge string.

-Dmitry



On 2015-05-20 05:54, Mandy Chung wrote:
 
 On May 18, 2015, at 5:17 AM, Dmitry Samersoff
 dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com wrote:

 Please review updated version of the fix:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/

 Most important part of the fix provided by Peter Levart, so all
 credentials belongs to him.
 
 
 My apology for being late to the review.  The subject line doesn’t catch
 my attention early enough :)
 
 I have to do further detail review tomorrow or so to follow the
 discussion and closely inspect the reference implementation.  Just to
 give you a quick comment.  I’m okay to add ReferenceQueue.forEach method
 at the first glance.  However I have trouble for
 Finalizer.printFinalizationQueue method that doesn’t belong there.  What
 are the other alternatives you have explored?
 
 Mandy
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Peter,

 What about creating a special package-private
 java.lang.ref.DiagnosticCommands class

I'm not quite happy with current printFinalizationQueue method - love to
have a way to print directly to DCMD pipe from Java rather than return a
huge string to VM.

But lang.ref.Finalizer is cached by VM (see
SystemDictionary::Finalizer_klass()) and is known as special
i.e. check-when-modify-hotspot class.

We don't plan to expand this DCMD so I'm reluctant to create a separate
class for one simple static method - it adds extra cost of
compiling/loading/care.

-Dmitry

On 2015-05-20 11:22, Peter Levart wrote:
 
 
 On 05/20/2015 08:51 AM, Dmitry Samersoff wrote:
 Mandy,

 However I have trouble for
 Finalizer.printFinalizationQueue method that doesn’t belong there.
 What are the other alternatives you have explored?
 Other alternatives could be to do all hashing/sorting/printing on native
 layer i.e. implement printFinalizationQueue inside VM.

 Both options has pros and cons - Java based solution requires less JNI
 calls and better readable but takes more memory.

 It might be better to return an array of Map.EntryString, int[]
 objects to VM rather than one huge string.

 -Dmitry
 
 Hi Dmitry,
 
 What about creating a special package-private
 java.lang.ref.DiagnosticCommands class which is then used as the home of
 static printFinalizationQueue method (and possible future diagnostic
 commands methods in the package). You could then expose a static
 package-private method from Finalizer:
 
 static void forEachEnqueued(Consumer? super Reference? action) {
 queue.forEach(action);
 }
 
 ...and use it to implement the printFinalizationQueue.
 
 Regards, Peter
 
 



 On 2015-05-20 05:54, Mandy Chung wrote:
 On May 18, 2015, at 5:17 AM, Dmitry Samersoff
 dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com
 wrote:

 Please review updated version of the fix:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/

 Most important part of the fix provided by Peter Levart, so all
 credentials belongs to him.

 My apology for being late to the review.  The subject line doesn’t catch
 my attention early enough :)

 I have to do further detail review tomorrow or so to follow the
 discussion and closely inspect the reference implementation.  Just to
 give you a quick comment.  I’m okay to add ReferenceQueue.forEach method
 at the first glance.  However I have trouble for
 Finalizer.printFinalizationQueue method that doesn’t belong there.  What
 are the other alternatives you have explored?

 Mandy


 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Staffan,

On 2015-05-20 14:19, Staffan Larsen wrote:
 Dmitry,
 
 I’ve look at the changes on the hotspot side.
 
 vm/services/diagnosticCommand.hpp:
 
  270   static const char* impact() {
  271 return Low;
  272   }
 
 I wonder if the impact should be “Medium” instead. There aren’t any good 
 guidelines for what impact means, but finalizerinfo does have non-negible 
 impact.

OK. I'll change it.

 
 
 test/serviceability/dcmd/gc/FinalizerInfoTest.java:
 
  69 while(wasInitialized != objectsCount);
 
 I don’t get the point of this loop. wasInitialized will always be equal to 
 objectsCount at this point.

Agree. It left from one of previous experiments.

 
  72 while(wasTrapped  1);
 
 Perhaps the System.gc() call should be inside this loop?

System.gc() instruct hotspot to start GC thread, but this thread may not
start immediately.

We need this loop to wait for GC thread.

 test/serviceability/dcmd/gc/HeapInfoTest.java:
 
 Can you add some output verification here?

This DCMD calls Universe::heap()-print_on() and output of this command
should be covered by GC tests, more complicated than this one, because
actual output depends to GC and heap parameters.

I can just check for presence of Metaspace world in the DCMD output.

-Dmitry

 
 
 /Staffan
 
 
 On 18 maj 2015, at 14:17, Dmitry Samersoff dmitry.samers...@oracle.com 
 wrote:

 Everyone,

 Please review updated version of the fix:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/

 Most important part of the fix provided by Peter Levart, so all
 credentials belongs to him.

 -Dmitry

 On 2015-05-16 15:48, Peter Levart wrote:


 On 05/16/2015 02:38 PM, Peter Levart wrote:


 On 05/16/2015 09:35 AM, Dmitry Samersoff wrote:
 Derek,

 Personally, I'm for volatile over explicit fence too.

 So I'll change the code if Peter don't have objections.

 There are no objections. There's one unneeded volatile store to .next
 field then in enqueue(), but it is followed by another volatile store,
 so there shouldn't be overhead on Intel and SPARC at least.

 Regards, Peter

 If you make .next field volatile, then perhaps you could also cache it's
 value in reallyPoll() into a local variable, so that it is not read
 twice. Like this:

private Reference? extends T reallyPoll() {   /* Must hold lock */
Reference? extends T r = head;
if (r != null) {
Reference rn = r.next; // HERE !!!
head = (rn == r) ?
null :
rn; // Unchecked due to the next field having a raw type
 in Reference
r.queue = NULL;
r.next = r;
queueLength--;
if (r instanceof FinalReference) {
sun.misc.VM.addFinalRefCount(-1);
}
return r;
}
return null;
}



 Peter



 -Dmitry

 On 2015-05-16 01:59, Derek White wrote:
 Hi Dmitry, Peter,

 I should read my email more frequently - I missed Dmitry's last webrev
 email.

 My comments on a preference for volatile over Unsafe are not a strong
 objection to using Unsafe fences. I just don't have enough experience
 with Unsafe fences yet to agree that this use is correct.

 While looking over this Reference code I found 3-6 really simple things
 that need cleaning up that have been there for years, so I'm not a big
 cheerleader for more complicated things here :-)

 - Derek

 On 5/15/15 6:46 PM, Derek White wrote:
 Hi Peter,

 Some comments below...

 On 5/14/15 3:50 AM, Peter Levart wrote:
 Hi Derek,

 On 05/13/2015 10:34 PM, Derek White wrote:
 Hi Peter,

 I don't have smoking gun evidence that your change introduces a bug,
 but I have some concerns...
 I did have a concern too, ...

 On 5/12/15 6:05 PM, Peter Levart wrote:
 Hi Dmitry,

 You iterate the queue then, not the unfinalized list. That's more
 logical.

 Holding the queue's lock may pause reference handler and finalizer
 threads for the entire time of iteration. This can blow up the
 application. Suppose one wants to diagnose the application because
 he suspects that finalizer thread hardly keeps up with production
 of finalizable instances. This can happen if the allocation rate of
 finalizable objects is very high and/or finalize() methods are not
 coded to be fast enough. Suppose the queue of Finalizer(s) builds
 up gradually and is already holding several million objects. Now
 you initiate the diagnostic command to print the queue. The
 iteration over and grouping/counting of several millions of
 Finalizer(s) takes some time. This blocks finalizer thread and
 reference handler thread. But does not block the threads that
 allocate finalizable objects. By the time the iteration is over,
 the JVM blows up and application slows down to a halt doing GCs
 most of the time, getting OOMEs, etc...

 It is possible to iterate the elements of the queue for diagnostic
 purposes without holding a lock. The modification required to do
 that is the following (havent tested this, but I

Re: RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Peter,

 I see diagnostic commands mostly format their output in native code. 
But for some of them (like this finalizer histogram) it would be nice 
to have a Java wrapper for hotspot's outputStream.

I love to have an ability to write pure-java DCMD's without touching of
hotspot code but it's a long term goal, out of scope of this fix.

Just a wrapper around hotspot output stream has a couple of underwear
complications around memory management and error handling, so it should
be a separate project.

-Dmitry


On 2015-05-20 14:44, Peter Levart wrote:
 
 
 On 05/20/2015 10:42 AM, Dmitry Samersoff wrote:
 Peter,

 What about creating a special package-private
 java.lang.ref.DiagnosticCommands class
 I'm not quite happy with current printFinalizationQueue method - love to
 have a way to print directly to DCMD pipe from Java rather than return a
 huge string to VM.

 But lang.ref.Finalizer is cached by VM (see
 SystemDictionary::Finalizer_klass()) and is known as special
 i.e. check-when-modify-hotspot class.

 We don't plan to expand this DCMD so I'm reluctant to create a separate
 class for one simple static method - it adds extra cost of
 compiling/loading/care.

 -Dmitry
 
 Ok then.
 
 I see diagnostic commands mostly format their output in native code. But
 for some of them (like this finalizer histogram) it would be nice to
 have a Java wrapper for hotspot's outputStream. It wouldn't be difficult
 to create such a class with JNI bindings, but where should one put it?
 Into which module?
 
 Otherwise, the simplest unformated data structure to return from such a
 method is an Object[] where you have String/Integer objects intermingled
 in pairs. Returning a Map.EntryString,int[][] is already more
 complicated to iterate and navigate in native code. MapString,int[]
 even more so.
 
 Regards, Peter
 

 On 2015-05-20 11:22, Peter Levart wrote:

 On 05/20/2015 08:51 AM, Dmitry Samersoff wrote:
 Mandy,

 However I have trouble for
 Finalizer.printFinalizationQueue method that doesn’t belong there.
 What are the other alternatives you have explored?
 Other alternatives could be to do all hashing/sorting/printing on
 native
 layer i.e. implement printFinalizationQueue inside VM.

 Both options has pros and cons - Java based solution requires less JNI
 calls and better readable but takes more memory.

 It might be better to return an array of Map.EntryString, int[]
 objects to VM rather than one huge string.

 -Dmitry
 Hi Dmitry,

 What about creating a special package-private
 java.lang.ref.DiagnosticCommands class which is then used as the home of
 static printFinalizationQueue method (and possible future diagnostic
 commands methods in the package). You could then expose a static
 package-private method from Finalizer:

 static void forEachEnqueued(Consumer? super Reference? action) {
  queue.forEach(action);
 }

 ...and use it to implement the printFinalizationQueue.

 Regards, Peter




 On 2015-05-20 05:54, Mandy Chung wrote:
 On May 18, 2015, at 5:17 AM, Dmitry Samersoff
 dmitry.samers...@oracle.com mailto:dmitry.samers...@oracle.com
 wrote:

 Please review updated version of the fix:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/

 Most important part of the fix provided by Peter Levart, so all
 credentials belongs to him.
 My apology for being late to the review.  The subject line doesn’t
 catch
 my attention early enough :)

 I have to do further detail review tomorrow or so to follow the
 discussion and closely inspect the reference implementation.  Just to
 give you a quick comment.  I’m okay to add ReferenceQueue.forEach
 method
 at the first glance.  However I have trouble for
 Finalizer.printFinalizationQueue method that doesn’t belong there. 
 What
 are the other alternatives you have explored?

 Mandy


 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(M,v7): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Everyone,

Please review updated version of the fix:

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.07/

Most important part of the fix provided by Peter Levart, so all
credentials belongs to him.

-Dmitry

On 2015-05-16 15:48, Peter Levart wrote:
 
 
 On 05/16/2015 02:38 PM, Peter Levart wrote:


 On 05/16/2015 09:35 AM, Dmitry Samersoff wrote:
 Derek,

 Personally, I'm for volatile over explicit fence too.

 So I'll change the code if Peter don't have objections.

 There are no objections. There's one unneeded volatile store to .next
 field then in enqueue(), but it is followed by another volatile store,
 so there shouldn't be overhead on Intel and SPARC at least.

 Regards, Peter
 
 If you make .next field volatile, then perhaps you could also cache it's
 value in reallyPoll() into a local variable, so that it is not read
 twice. Like this:
 
 private Reference? extends T reallyPoll() {   /* Must hold lock */
 Reference? extends T r = head;
 if (r != null) {
 Reference rn = r.next; // HERE !!!
 head = (rn == r) ?
 null :
 rn; // Unchecked due to the next field having a raw type
 in Reference
 r.queue = NULL;
 r.next = r;
 queueLength--;
 if (r instanceof FinalReference) {
 sun.misc.VM.addFinalRefCount(-1);
 }
 return r;
 }
 return null;
 }
 
 
 
 Peter
 
 

 -Dmitry

 On 2015-05-16 01:59, Derek White wrote:
 Hi Dmitry, Peter,

 I should read my email more frequently - I missed Dmitry's last webrev
 email.

 My comments on a preference for volatile over Unsafe are not a strong
 objection to using Unsafe fences. I just don't have enough experience
 with Unsafe fences yet to agree that this use is correct.

 While looking over this Reference code I found 3-6 really simple things
 that need cleaning up that have been there for years, so I'm not a big
 cheerleader for more complicated things here :-)

  - Derek

 On 5/15/15 6:46 PM, Derek White wrote:
 Hi Peter,

 Some comments below...

 On 5/14/15 3:50 AM, Peter Levart wrote:
 Hi Derek,

 On 05/13/2015 10:34 PM, Derek White wrote:
 Hi Peter,

 I don't have smoking gun evidence that your change introduces a bug,
 but I have some concerns...
 I did have a concern too, ...

 On 5/12/15 6:05 PM, Peter Levart wrote:
 Hi Dmitry,

 You iterate the queue then, not the unfinalized list. That's more
 logical.

 Holding the queue's lock may pause reference handler and finalizer
 threads for the entire time of iteration. This can blow up the
 application. Suppose one wants to diagnose the application because
 he suspects that finalizer thread hardly keeps up with production
 of finalizable instances. This can happen if the allocation rate of
 finalizable objects is very high and/or finalize() methods are not
 coded to be fast enough. Suppose the queue of Finalizer(s) builds
 up gradually and is already holding several million objects. Now
 you initiate the diagnostic command to print the queue. The
 iteration over and grouping/counting of several millions of
 Finalizer(s) takes some time. This blocks finalizer thread and
 reference handler thread. But does not block the threads that
 allocate finalizable objects. By the time the iteration is over,
 the JVM blows up and application slows down to a halt doing GCs
 most of the time, getting OOMEs, etc...

 It is possible to iterate the elements of the queue for diagnostic
 purposes without holding a lock. The modification required to do
 that is the following (havent tested this, but I think it should work):


 http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.01/

 One issue to watch out for is the garbage collectors inspect the
 Reference.next field from C code directly (to distinguish active vs.
 pending, iterate over oops, etc.). You can search hotspot for
 java_lang_ref_Reference::next*, java_lang_ref_Reference::set_next*.

 Your change makes inactive References superficially look like
 enqueued References. The GC code is rather subtle and I haven't
 yet seen a case where it would get confused by this change, but
 there could easily be something like that lurking in the GC code.
 ...but then I thought that GC can in no way treat a Reference
 differently whether it is still enqueued in a ReferenceQueue or
 already dequeued (inactive) - responsibility is already on the Java
 side. Currently the definition of Reference.next is this:

 /* When active:   NULL
  * pending:   this
  *Enqueued:   next reference in queue (or this if last)
  *Inactive:   this
  */
 @SuppressWarnings(rawtypes)
 Reference next;

 We see that, unless GC inspects all ReferenceQueue instances and
 scans their lists too, the state of a Reference that is enqueued as
 last in list is indistinguishable from the state of inactive
 Reference. So I deduced that this distinction

Re: RFR(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

() might see inconsistent values for the queue
 and next fields of a Reference, but we don't want to grab a lock
 walking the whole queue, we could instead grab the lock as we look at
 each Reference (and do the back-up trick if we were interfered
 with). Of course this is slower than going lock-free, but it's not any
 more overhead than the ReferenceHandler thread or FinalizerThreads incur.

 The only benefit of this solution over the others is that it is less
 clever, and I'm not sure how much cleverness this problem deserves.
 Given that, and ranking the solutions as lock  (clever) volatile
  fence, I'd be happier with the volatile or lock solution if
 that is sufficient.

  - Derek

 I also suggest the addition to the ReferenceQueue to be contained
 (package-private) and as generic as possible. That's why I suggest
 forEach iteration method with no concrete logic.

 It would be possible to encapsulate the entire logic into a special
 package-private class (say java.lang.ref.DiagnosticCommands) with
 static method(s) (printFinalizationQueue)... You could just expose
 a package-private forEach static method from Finalizer and code the
 rest in DiagnosticCommands.
 That's good for encapsulation. But I'm concerned that if forEach
 got exposed beyond careful use in DiagnosticCommands, and the
 Referents were leaked back into the heap, then we could get
 unexpected object resurrection after finalization. This isn't a bug
 on it's own - any finalizer might resurrect its object if not
 careful, but ordinarily /only/ the finalizer could resurrect the
 object. This could invalidate application invariants?

 Well, it all stays in the confines of package-private API - internal
 to JDK. Any future additional use should be reviewed carefully.
 Comments on the forEach() method should warn about that.


 I agree that using a lock over the ReferenceQueue iteration opens up
 /another/ case of diagnostics affecting application behavior. And
 there are pathological scenarios where this gets severe. This is
 unfortunate but not uncommon. There is enough complication here that
 you should be sure that the fix for diagnostics performance doesn't
 introduce subtle bugs to the system in general. A change in this
 area should get a thorough review from both the runtime and GC sides.

 Is the webrev.02 proposed above more acceptable in that respect? It
 does not introduce any logical changes to existing code.


 Better yet, the reference handling code in GC and runtime should
 probably be thrown out and re-written, but that's another issue :-)

 I may have some proposals in that direction. Stay tuned.

 Regards, Peter


 - Derek

 Regards, Peter


 On 05/12/2015 07:10 PM, Dmitry Samersoff wrote:
 Everybody,

 Updated version:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.03/

 Now it iterates over queue and output result sorted by number of 
 instances.

 -Dmitry

 On 2015-05-07 00:51, Derek White wrote:
 Hi Dmitry, Staffan,

 Lots of good comments here.

 On the topic of what list should be printed out, I think we should focus
 on objects waiting to be finalized - e.g. the contents of the
 ReferenceQueue. It's more of a pain to walk the ReferenceQueue, but you
 could add a summerizeQueue(TreeMapString, Integer) method, or a
 general iterator and lambda.

 A histogram of objects with finalize methods might also be interesting,
 but you can get most of the same information from a heap histogram
 (ClassHistogramDCmd, and search for count of Finalizer instances).

 BTW, by only locking the ReferenceQueue, we should only be blocking the
 FinalizerThread from making progress (and I think there's a GC thread
 that runs after GC that sorts found References objects that need
 processing into their respective ReferenceQueues). But locking the
 unfinalized list blocks initializing any object with a finalize() 
 method.

 The sorting suggestion is a nice touch.

  - Derek White, GC team

 On 5/5/15 10:40 AM, Peter Levart wrote:
 Hi Dmitry, Staffan,

 On 05/05/2015 12:38 PM, Staffan Larsen wrote:
 Dmitry,

 I think this should be reviewed on hotspot-gc and core-libs-dev as
 well considering the changes to Finalizer.

 I’m a little worried about the potentially very large String that is
 returned from printFinalizationQueue(). A possible different approach
 would be to write printFinalizationQueue() in C++ inside Hotspot.
 That would allow for outputting one line at a time. The output would
 still be saved in memory (since the stream is buffered), but at least
 the data is only saved once in memory, then. It would make the code a
 bit harder to write, so its a question of how scared we are of
 running out of memory.
 If the output is just a histogram of # of instances per class name,
 then it should not be too large, as there are not many different
 classes overriding finalize() method (I counted 72 overriddings of
 finalize() method in the whole jdk/src tree).

 I'm more concerned about the fact that while traversing the list

Re: RFR(M,v6): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Everybody,

Please review updated version.

http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.06/

JDK part provided by Peter Levart, so all credentials belongs to him.

-Dmitry

On 2015-05-14 23:35, Peter Levart wrote:
 
 
 On 05/14/2015 10:11 PM, Peter Levart wrote:
 Hi Dmitry,

 ReferenceQueue is not really a queue, but a LIFO stack. Scanner is
 walking the stack from top (the 'head') to bottom (the last element
 pointing to itself). When poller(s) overtake the scanner, it actually
 means that the top of the stack has been eaten under scanner's feet
 while trying to grab it. Restarting from 'head' actually means
 'catching-up' wit poller(s) and trying to keep up with them. We don't
 get the 'eaten' elements, but might be lucky to get some more food
 until it's all eaten. Usually we will get all of the elements, since
 poller(s) must synchronize and do additional work, so they are slower
 and there's also enqueuer(s) that push elements on the top of the
 stack so poller(s) must eat those last pushed elements before they can
 continue chasing the scanner...

 When scanner is overtaken by poller, then there is a chance the
 scanner will get less elements than there were present in the stack if
 a snapshot was taken, so catching-up by restarting from 'head' tries
 to at least recover some of the rest of the elements of that snapshot
 before they are gone.
 
 Also, the chance that the scanner is overtaken by poller is greater at
 the start of race. The scanner and poller start from the same spot and
 as we know threads are jumpy so it will happen quite frequently that a
 poller jumps before scanner. So just giving-up when overtaken might
 return 0 or just a few elements when there are millions there in the
 queue. When scanner finally gets a head start, it will usually lead the
 race to the end.
 
 Peter
 

 Does this make more sense now?

 Regards, Peter

 On 05/14/2015 07:41 PM, Dmitry Samersoff wrote:
 Peter,

 Could we just bail out on r == r.next?

 It gives us less accurate result, but I suspect that If we restart from
 head we need to flush all counters.

 As far I understand queue poller removes items one by one from a queue
 end so if we overtaken by queue poller we can safely assume that
 we are at the end of the queue.

 Is it correct?

 -Dmitry

 On 2015-05-14 10:50, Peter Levart wrote:
 Hi Derek,

 On 05/13/2015 10:34 PM, Derek White wrote:
 Hi Peter,

 I don't have smoking gun evidence that your change introduces a bug,
 but I have some concerns...
 I did have a concern too, ...

 On 5/12/15 6:05 PM, Peter Levart wrote:
 Hi Dmitry,

 You iterate the queue then, not the unfinalized list. That's more
 logical.

 Holding the queue's lock may pause reference handler and finalizer
 threads for the entire time of iteration. This can blow up the
 application. Suppose one wants to diagnose the application because he
 suspects that finalizer thread hardly keeps up with production of
 finalizable instances. This can happen if the allocation rate of
 finalizable objects is very high and/or finalize() methods are not
 coded to be fast enough. Suppose the queue of Finalizer(s) builds up
 gradually and is already holding several million objects. Now you
 initiate the diagnostic command to print the queue. The iteration
 over and grouping/counting of several millions of Finalizer(s) takes
 some time. This blocks finalizer thread and reference handler thread.
 But does not block the threads that allocate finalizable objects. By
 the time the iteration is over, the JVM blows up and application
 slows down to a halt doing GCs most of the time, getting OOMEs, etc...

 It is possible to iterate the elements of the queue for diagnostic
 purposes without holding a lock. The modification required to do that
 is the following (havent tested this, but I think it should work):


 http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.01/

 One issue to watch out for is the garbage collectors inspect the
 Reference.next field from C code directly (to distinguish active vs.
 pending, iterate over oops, etc.). You can search hotspot for
 java_lang_ref_Reference::next*, java_lang_ref_Reference::set_next*.

 Your change makes inactive References superficially look like
 enqueued References. The GC code is rather subtle and I haven't yet
 seen a case where it would get confused by this change, but there
 could easily be something like that lurking in the GC code.
 ...but then I thought that GC can in no way treat a Reference
 differently whether it is still enqueued in a ReferenceQueue or already
 dequeued (inactive) - responsibility is already on the Java side.
 Currently the definition of Reference.next is this:

 /* When active:   NULL
  * pending:   this
  *Enqueued:   next reference in queue (or this if last)
  *Inactive:   this
  */
 @SuppressWarnings(rawtypes)
 Reference next;

 We see that, unless GC inspects all ReferenceQueue instances

Re: RFR(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

, then we could get unexpected object
 resurrection after finalization. This isn't a bug on it's own - any
 finalizer might resurrect its object if not careful, but ordinarily
 /only/ the finalizer could resurrect the object. This could invalidate
 application invariants?
 
 Well, it all stays in the confines of package-private API - internal to
 JDK. Any future additional use should be reviewed carefully. Comments on
 the forEach() method should warn about that.
 

 I agree that using a lock over the ReferenceQueue iteration opens up
 /another/ case of diagnostics affecting application behavior. And
 there are pathological scenarios where this gets severe. This is
 unfortunate but not uncommon. There is enough complication here that
 you should be sure that the fix for diagnostics performance doesn't
 introduce subtle bugs to the system in general. A change in this area
 should get a thorough review from both the runtime and GC sides.
 
 Is the webrev.02 proposed above more acceptable in that respect? It does
 not introduce any logical changes to existing code.
 

 Better yet, the reference handling code in GC and runtime should
 probably be thrown out and re-written, but that's another issue :-)
 
 I may have some proposals in that direction. Stay tuned.
 
 Regards, Peter
 

 - Derek

 Regards, Peter


 On 05/12/2015 07:10 PM, Dmitry Samersoff wrote:
 Everybody,

 Updated version:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.03/

 Now it iterates over queue and output result sorted by number of instances.

 -Dmitry

 On 2015-05-07 00:51, Derek White wrote:
 Hi Dmitry, Staffan,

 Lots of good comments here.

 On the topic of what list should be printed out, I think we should focus
 on objects waiting to be finalized - e.g. the contents of the
 ReferenceQueue. It's more of a pain to walk the ReferenceQueue, but you
 could add a summerizeQueue(TreeMapString, Integer) method, or a
 general iterator and lambda.

 A histogram of objects with finalize methods might also be interesting,
 but you can get most of the same information from a heap histogram
 (ClassHistogramDCmd, and search for count of Finalizer instances).

 BTW, by only locking the ReferenceQueue, we should only be blocking the
 FinalizerThread from making progress (and I think there's a GC thread
 that runs after GC that sorts found References objects that need
 processing into their respective ReferenceQueues). But locking the
 unfinalized list blocks initializing any object with a finalize() 
 method.

 The sorting suggestion is a nice touch.

  - Derek White, GC team

 On 5/5/15 10:40 AM, Peter Levart wrote:
 Hi Dmitry, Staffan,

 On 05/05/2015 12:38 PM, Staffan Larsen wrote:
 Dmitry,

 I think this should be reviewed on hotspot-gc and core-libs-dev as
 well considering the changes to Finalizer.

 I’m a little worried about the potentially very large String that is
 returned from printFinalizationQueue(). A possible different approach
 would be to write printFinalizationQueue() in C++ inside Hotspot.
 That would allow for outputting one line at a time. The output would
 still be saved in memory (since the stream is buffered), but at least
 the data is only saved once in memory, then. It would make the code a
 bit harder to write, so its a question of how scared we are of
 running out of memory.
 If the output is just a histogram of # of instances per class name,
 then it should not be too large, as there are not many different
 classes overriding finalize() method (I counted 72 overriddings of
 finalize() method in the whole jdk/src tree).

 I'm more concerned about the fact that while traversing the list, a
 lock is held, which might impact prompt finalization(). Is it
 acceptable for diagnostic output to impact performance and/or
 interfere with synchronization?

 It might be possible to scan the list without holding a lock for
 diagnostic purposes if Finalizer.remove() didn't set the removed
 Finalizer.next pointer to point back to itself:

 private void remove() {
 synchronized (lock) {
 if (unfinalized == this) {
 if (this.next != null) {
 unfinalized = this.next;
 } else {
 unfinalized = this.prev;
 }
 }
 if (this.next != null) {
 this.next.prev = this.prev;
 }
 if (this.prev != null) {
 this.prev.next = this.next;
 }
 // this.next = this; must not be set so that we can
 traverse the list unsynchronized
 this.prev = this;   /* Indicates that this has been
 finalized */
 }
 }

 For detecting whether a Finalizer is already processed, the 'prev'
 pointer could be used instead:

 private boolean hasBeenFinalized() {
 return (prev == this);
 }

 Also, to make sure a data race dereferencing 'unfinalized' in
 unsynchronized printFinalizationQueue() would get you a fully

Re: RFR(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

Peter,

Thank you for the explanation!

-Dmitry

On 2015-05-14 23:35, Peter Levart wrote:
 
 
 On 05/14/2015 10:11 PM, Peter Levart wrote:
 Hi Dmitry,

 ReferenceQueue is not really a queue, but a LIFO stack. Scanner is
 walking the stack from top (the 'head') to bottom (the last element
 pointing to itself). When poller(s) overtake the scanner, it actually
 means that the top of the stack has been eaten under scanner's feet
 while trying to grab it. Restarting from 'head' actually means
 'catching-up' wit poller(s) and trying to keep up with them. We don't
 get the 'eaten' elements, but might be lucky to get some more food
 until it's all eaten. Usually we will get all of the elements, since
 poller(s) must synchronize and do additional work, so they are slower
 and there's also enqueuer(s) that push elements on the top of the
 stack so poller(s) must eat those last pushed elements before they can
 continue chasing the scanner...

 When scanner is overtaken by poller, then there is a chance the
 scanner will get less elements than there were present in the stack if
 a snapshot was taken, so catching-up by restarting from 'head' tries
 to at least recover some of the rest of the elements of that snapshot
 before they are gone.
 
 Also, the chance that the scanner is overtaken by poller is greater at
 the start of race. The scanner and poller start from the same spot and
 as we know threads are jumpy so it will happen quite frequently that a
 poller jumps before scanner. So just giving-up when overtaken might
 return 0 or just a few elements when there are millions there in the
 queue. When scanner finally gets a head start, it will usually lead the
 race to the end.
 
 Peter
 

 Does this make more sense now?

 Regards, Peter

 On 05/14/2015 07:41 PM, Dmitry Samersoff wrote:
 Peter,

 Could we just bail out on r == r.next?

 It gives us less accurate result, but I suspect that If we restart from
 head we need to flush all counters.

 As far I understand queue poller removes items one by one from a queue
 end so if we overtaken by queue poller we can safely assume that
 we are at the end of the queue.

 Is it correct?

 -Dmitry

 On 2015-05-14 10:50, Peter Levart wrote:
 Hi Derek,

 On 05/13/2015 10:34 PM, Derek White wrote:
 Hi Peter,

 I don't have smoking gun evidence that your change introduces a bug,
 but I have some concerns...
 I did have a concern too, ...

 On 5/12/15 6:05 PM, Peter Levart wrote:
 Hi Dmitry,

 You iterate the queue then, not the unfinalized list. That's more
 logical.

 Holding the queue's lock may pause reference handler and finalizer
 threads for the entire time of iteration. This can blow up the
 application. Suppose one wants to diagnose the application because he
 suspects that finalizer thread hardly keeps up with production of
 finalizable instances. This can happen if the allocation rate of
 finalizable objects is very high and/or finalize() methods are not
 coded to be fast enough. Suppose the queue of Finalizer(s) builds up
 gradually and is already holding several million objects. Now you
 initiate the diagnostic command to print the queue. The iteration
 over and grouping/counting of several millions of Finalizer(s) takes
 some time. This blocks finalizer thread and reference handler thread.
 But does not block the threads that allocate finalizable objects. By
 the time the iteration is over, the JVM blows up and application
 slows down to a halt doing GCs most of the time, getting OOMEs, etc...

 It is possible to iterate the elements of the queue for diagnostic
 purposes without holding a lock. The modification required to do that
 is the following (havent tested this, but I think it should work):


 http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.01/

 One issue to watch out for is the garbage collectors inspect the
 Reference.next field from C code directly (to distinguish active vs.
 pending, iterate over oops, etc.). You can search hotspot for
 java_lang_ref_Reference::next*, java_lang_ref_Reference::set_next*.

 Your change makes inactive References superficially look like
 enqueued References. The GC code is rather subtle and I haven't yet
 seen a case where it would get confused by this change, but there
 could easily be something like that lurking in the GC code.
 ...but then I thought that GC can in no way treat a Reference
 differently whether it is still enqueued in a ReferenceQueue or already
 dequeued (inactive) - responsibility is already on the Java side.
 Currently the definition of Reference.next is this:

 /* When active:   NULL
  * pending:   this
  *Enqueued:   next reference in queue (or this if last)
  *Inactive:   this
  */
 @SuppressWarnings(rawtypes)
 Reference next;

 We see that, unless GC inspects all ReferenceQueue instances and scans
 their lists too, the state of a Reference that is enqueued as last in
 list is indistinguishable from the state of inactive

Re: RFR(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

 that are still reachable and alive). The later serves
 two purposes:
 - it keeps Finalizer instances reachable until they are processed
 - it is a source of unfinalized instances for
 running-finalizers-on-exit if requested by
 System.runFinalizersOnExit(true);

 So it really depends on what one would like to see. Showing the queue
 may be interesting if one wants to see how the finalizer thread is
 coping with processing the finalize() invocations. Showing unfinalized
 list may be interesting if one wants to know how many live +
 finalization pending instances are there on the heap that override
 finalize() method.

 Regards, Peter


 For the output, it would be a nice touch to sort it on the number of
 references for each type. Perhaps outputting it more like a table
 (see the old code again) would also make it easier to digest.

 In diagnosticCommand.hpp, the new commands should override
 permission() and limit access:

static const JavaPermission permission() {
  JavaPermission p = {java.lang.management.ManagementPermission,
  monitor, NULL};
  return p;
}

 The two tests don’t validate the output in any way. Would it be
 possible to add validation? Perhaps hard to make sure an object is on
 the finalizer queue… Hmm.

 Thanks,
 /Staffan


 On 5 maj 2015, at 12:01, Dmitry Samersoff
 dmitry.samers...@oracle.com wrote:

 Everyone,

 Please review the fix:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8059036/webrev.01/

 heap dcmd outputs the same information as SIGBREAK

 finalizerinfo dcmd outputs a list of all classes in finalization queue
 with count

 -Dmitry

 -- 
 Dmitry Samersoff
 Oracle Java development team, Saint Petersburg, Russia
 * I would love to change the world, but they won't give me the sources.

 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: [9] RFR 8074840: Resolve disabled warnings for libjli and libjli_static

[Dmitry Samersoff]

Mikael,

Thank you for doing it!

Looks good for me (fill free to ignore comments below).


splashscreen_stubs.c
java_md_macosx.c:

855: it might be better to use rslt directly

parse_manifest.c:

196: one pf possible solution is declare len as unsigned int and cast it
to (jlong) at 192 if necessary

-Dmitry




On 2015-03-20 21:02, Mikael Vidstedt wrote:
 
 Please review the following change which fixes a number of native
 compilation warnings in files related to libjli.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8074840
 Webrev:
 http://cr.openjdk.java.net/~mikael/webrevs/8074840/webrev.01/webrev/
 
 I built the code across all the OracleJDK platforms and there were no
 warnings on any of the platforms (in the files related to this change).
 I'm taking suggestions on specific tests to run.
 
 Some specifics:
 
 Unfortunately there is no intrinsic for cpuid in Solaris Studio, so I
 had to keep the inline asm code and the corresponding flag to disable
 the related warning (E_ASM_DISABLES_OPTIMIZATION). The alternative would
 be to move it out into a dedicated assembly file, but that seems like
 more trouble than it's worth given that the warning is harmless.
 
 I'm not entirely happy with the unsigned cast in parse_manifest.c:196,
 but unfortunately the windows prototype for read() takes an unsigned as
 its last argument, where All(tm) other platforms take a size_t. In this
 specific case 'len' will never be be more than END_MAXLEN = 0x +
 ENDHDR = 0x + 22 = 0x10015, meaning it will comfortably fit in an
 unsigned on the platforms we support. But if somebody has suggestions
 I'm all ears, doing the #ifdef dance is of course also an option.
 
 Note that the warnings were temporarily disabled as part of JDK-8074096
 [1] until such time they could be fixed the proper way. Also note that
 this change supersedes the earlier change [2] Dmitry had out for review.
 The bug [3] corresponding to that webrev will be closed as a duplicate
 of this bug (JDK-8074839).
 
 Cheers,
 Mikael
 
 [1] https://bugs.openjdk.java.net/browse/JDK-8074096
 [2] http://cr.openjdk.java.net/~dsamersoff/JDK-8073175/webrev.01/
 [3] https://bugs.openjdk.java.net/browse/JDK-8073175
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.

Re: RFR(S): JDK-8073584 Native compilation warning in unpack code

[Dmitry Samersoff]

David at all,

May I consider the fix as reviewed and continue with integration?

-Dmitry


On 2015-02-24 05:23, David Holmes wrote:
 On 24/02/2015 12:02 AM, Dmitry Samersoff wrote:
 Hi Everyone,

 Webrev updated in-place (press shift-reload)

 http://cr.openjdk.java.net/~dsamersoff/JDK-8073584/webrev.01/
 
 share/native/libunpack/jni.cpp
 
 295 return (jobject) NULL;
 
 Why do you need a cast on NULL?
 
 Updated formatting.

 Hack in main.cpp replaced with true error check.
 
 Not sure it is appropriate to lump the res != 1 in with the CR error.
 Doesn't this case deserve its own u.abort(xxx) ?
 
 Thanks,
 David
 
 -Dmitry


 On 2015-02-23 05:07, David Holmes wrote:
 On 21/02/2015 4:27 AM, Dmitry Samersoff wrote:
 Hi Everyone,

 It's my $0.02 to the warning cleanup work. Please review:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8073584/webrev.01/

 Notes:

 I use an ugly trick: (void) (read() + 1) to get rid of ignored value
 warning because since gcc 4.6 just (void) is not enough.

 Why not just check the return value for correctness?

 David


 -Dmitry






-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: stop using mmap for zip I/O

[Dmitry Samersoff]

Christos,

JVM install it's own signal handler and use it to numerous tasks, so add
signal handler to zip_util.c is problematic.

The simplest way to address the problem is use mlock/munlock to test
page presence before touching it.

i.e. add code like

 if ((i % 4096) == 0) {
 if ( mlock(v+i, 4096)  0) {
   printf(No page. Bail out\n);
   return;
 }
  }

to *compute()* in your example below.


-Dmitry




On 2015-02-27 01:17, chris...@zoulas.com wrote:
 Hi,
 
 There are numerous bug reports about the jvm crashing in libzip...
 Just google for libzip java crash. The bottom line is that using
 mmap is problematic (I can get into more per OS details if necessary)
 because it will potentially signal when the file size is altered.
 Can we please turn USE_MMAP off, and/or remove the code (zip_util.c)?
 I don't think it is acceptable for the jvm to crash if it tries to
 read a file while it is being modified. The following simple program
 demonstrates the issue... just:
 
 $ cc mmap.c
 $ cp a.out b.out
 $ ./a.out b.out
 
 Best,
 
 christos
 
 $ cat  _EOF  mmap.c
 #include stdio.h
 #include stdlib.h
 #include unistd.h
 #include fcntl.h
 #include err.h
 #include signal.h
 
 #include sys/mman.h
 #include sys/stat.h
 
 volatile size_t i;
 size_t size = 0;
 
 void
 sig(int s)
 {
   printf(boom %d %zu\n, s, i);
   exit(1);
 }
 
 void
 compute(unsigned char *v)
 {
   int j = 0;
   for (i = 0; i  size; i++)
   j += v[i];
   printf(%d\n, j);
 }
 
 int
 main(int argc, char *argv[])
 {
   struct stat st;
   unsigned char *v;
   int fd;
 
   signal(SIGSEGV, sig);
   signal(SIGBUS, sig);
   fd = open(argv[1], O_RDONLY);
   if (fd == -1)
   err(1, open %s, argv[1]);
 
   if (fstat(fd, st) == -1)
   err(1, fstat %s, argv[1]);
   size = st.st_size;
   if (size == 0)
   errx(1, 0 sized file);
   
   v = mmap(0, size, PROT_READ, MAP_FILE | MAP_PRIVATE, fd, 0);
   if (v == MAP_FAILED)
   err(1, mmap);
 
   printf(go1\n);
   compute(v);
   truncate(argv[1], 0);
   printf(go2\n);
   compute(v);
   return 0;
 }
 _EOF
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S): JDK-8073584 Native compilation warning in unpack code

[Dmitry Samersoff]

Hi Everyone,

Webrev updated in-place (press shift-reload)

http://cr.openjdk.java.net/~dsamersoff/JDK-8073584/webrev.01/

Updated formatting.

Hack in main.cpp replaced with true error check.

-Dmitry


On 2015-02-23 05:07, David Holmes wrote:
 On 21/02/2015 4:27 AM, Dmitry Samersoff wrote:
 Hi Everyone,

 It's my $0.02 to the warning cleanup work. Please review:

 http://cr.openjdk.java.net/~dsamersoff/JDK-8073584/webrev.01/

 Notes:

 I use an ugly trick: (void) (read() + 1) to get rid of ignored value
 warning because since gcc 4.6 just (void) is not enough.
 
 Why not just check the return value for correctness?
 
 David
 

 -Dmitry




-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S): JDK-8073584 Native compilation warning in unpack code

[Dmitry Samersoff]

John,

Generally speaking, we should select warnings carefully - not just turn
all of it on.

For instance, I don't see any value of format is not a string literal
warning for JDK code.

Please, see also below.


On 2015-02-22 06:20, John Rose wrote:
 On Feb 21, 2015, at 5:38 PM, Kumar Srinivasan kumar.x.sriniva...@oracle.com 
 wrote:

 Hi Dmitry,

 adding John on this.

 unpack.cpp
 What is wrong with the  unary operator ? Does this emit a compiler warning ?
 
 (It's the ternary operator right?)  


 The problem is that the format string (oh, the cleverness!!) is non-constant.

Yes. Actually sprintf here could be replaced with just a strcpy.

 -  sprintf(buf, ((uint)e.tag  CONSTANT_Limit)? TAG_NAME[e.tag]: %d, 
 e.tag);
 +  if ((uint)e.tag  CONSTANT_Limit) {
 +sprintf(buf, %s, TAG_NAME[e.tag]);
 +  }
 +  else {
 +sprintf(buf, %d, e.tag);
 +  }

 If you are eliminating the unary operator then, the formatting should be

 if (.)
   ..
 else
   ..
 or
 if (.) {
 
 else {  --- note, please don't introduce new formatting/conventions.
 
 I agree on that.  Let's be whitespace chameleons.

I'll change it.

 
   
 }

 main.cpp:

 +  (void) (fread(filecrc, sizeof(filecrc), 1, u.infileptr) + 1);

 UGH. What about other compilers are they ok with this ? This may very well
 get suppressed for gcc, but might emit warnings on MSVC or SunStudio.

It works for all JDK compilers.

 
 Surely it would be better to bind the result of fread to a throwaway 
 variable, if there's a warning about unused return value.
 void* fread_result_ignored =
  fread(filecrc, sizeof(filecrc), 1, u.infileptr);


It causes assigned but unused variable warning.

-Dmitry

-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

RFR(S): JDK-8073584 Native compilation warning in unpack code

[Dmitry Samersoff]

Hi Everyone,

It's my $0.02 to the warning cleanup work. Please review:

http://cr.openjdk.java.net/~dsamersoff/JDK-8073584/webrev.01/

Notes:

I use an ugly trick: (void) (read() + 1) to get rid of ignored value
warning because since gcc 4.6 just (void) is not enough.


-Dmitry


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

RFR(S): JDK-8073175, 8073174: Fix native warnings in libjli and libfdlibm

[Dmitry Samersoff]

Hi Everyone,

It's my $0.02 to the warning cleanup work. Please review:

http://cr.openjdk.java.net/~dsamersoff/JDK-8073175/webrev.01
http://cr.openjdk.java.net/~dsamersoff/JDK-8073174/webrev.01

Notes:

1.
There are two common ways to cast pointer to int: intptr_t (perfectly
safe, more-or-less portable) and size_t (perfectly portable,
more-or-less safe)

So I use size_t in shared code, intptr_t in UNIX specific code.

2.
I didn't fix format not a string literal warnings. It requires to set
per-compiler pragmas.

-Dmitry


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8057746: Cannot handle JdpException in JMX agent initialization.

[Dmitry Samersoff]

Yasumasa,

I'll sponsor the push.

-Dmitry

On 2014-09-07 17:04, Yasumasa Suenaga wrote:
 Hi all,
 
 This issue is related to JDK-8057556: JDP should better handle non-active 
 interfaces
 http://mail.openjdk.java.net/pipermail/serviceability-dev/2014-September/015548.html
 
 JMX agent will be terminated silently when I run command as following:
 
 java -Dcom.sun.management.jmxremote.port=7091 
 -Dcom.sun.management.jmxremote.authenticate=false
  -Dcom.sun.management.jmxremote.ssl=false 
 -Dcom.sun.management.jmxremote.autodiscovery=true
  -Dcom.sun.management.jdp.source_addr=255.255.255.255 -version
 
 I expect JdpException which is caused by com.sun.management.jdp.source_addr 
 .
 
 Curently, sun.management.Agent#startDiscoveryService() throws 
 AgentConfigurationError
 when JdpException is occurred. However, argument of AgentConfiguratonError 
 constructor
 is incorrected. So NPE will be occurred. (I checked it with jdb.)
 
 I've created webrev. Could you review it?
 http://cr.openjdk.java.net/~ysuenaga/JDK-8057746/webrev.0/
 
 
 I would like to contribute this patch.
 Please review and sponsoring.
 
 
 Thanks,
 
 Yasumasa 
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.

Re: RFR: JDK-8057556: JDP should better handle non-active interfaces

[dmitry . samersoff]

Please, file a separate issue.

--Dmitry



-Original Message-
From: Yasumasa Suenaga yasue...@gmail.com
To: Peter Allwin peter.all...@oracle.com
Cc: Dmitry Samersoff dmitry.samers...@oracle.com, 
core-libs-dev@openjdk.java.net core-libs-dev@openjdk.java.net, 
serviceability-...@openjdk.java.net serviceability-...@openjdk.java.net
Sent: Sat, 06 Sep 2014 19:37
Subject: Re: RFR: JDK-8057556: JDP should better handle non-active interfaces

Hi all,

My patch works fine in my environment, however, JMX agent will be terminated
silently when I run command as following:

java -Dcom.sun.management.jmxremote.port=7091 
-Dcom.sun.management.jmxremote.authenticate=false
  -Dcom.sun.management.jmxremote.ssl=false 
-Dcom.sun.management.jmxremote.autodiscovery=true
  -Dcom.sun.management.jdp.source_addr=255.255.255.255 -version

Curently, sun.management.Agent#startDiscoveryService() throws 
AgentConfigurationError
when JdpException is occurred. However, argument of AgentConfiguratonError 
constructor
is incorrected. So NPE will be occurred. (I checked it with jdb.)

I think that we should fix it as following.

---
diff -r 68a6bb51cb26 src/java.management/share/classes/sun/management/Agent.java
--- a/src/java.management/share/classes/sun/management/Agent.java   Mon Sep 
01 13:33:28 2014 +0200
+++ b/src/java.management/share/classes/sun/management/Agent.java   Sat Sep 
06 23:50:58 2014 +0900
@@ -210,6 +210,8 @@
  } else {
  throw new AgentConfigurationError(INVALID_JMXREMOTE_PORT, No 
port specified);
  }
+} catch (JdpException e) {
+error(e);
  } catch (AgentConfigurationError err) {
  error(err.getError(), err.getParams());
  }
@@ -273,7 +275,7 @@
  }

  private static void startDiscoveryService(Properties props)
-throws IOException {
+throws IOException, JdpException {
  // Start discovery service if requested
  String discoveryPort = 
props.getProperty(com.sun.management.jdp.port);
  String discoveryAddress = 
props.getProperty(com.sun.management.jdp.address);
@@ -291,7 +293,7 @@
  try{
 shouldStart = Boolean.parseBoolean(discoveryShouldStart);
  } catch (NumberFormatException e) {
-throw new AgentConfigurationError(Couldn't parse 
autodiscovery argument);
+throw new AgentConfigurationError(AGENT_EXCEPTION, Couldn't 
parse autodiscovery argument);
  }
  }

@@ -302,7 +304,7 @@
  address = (discoveryAddress == null) ?
  InetAddress.getByName(JDP_DEFAULT_ADDRESS) : 
InetAddress.getByName(discoveryAddress);
  } catch (UnknownHostException e) {
-throw new AgentConfigurationError(Unable to broadcast to 
requested address, e);
+throw new AgentConfigurationError(AGENT_EXCEPTION, e, Unable 
to broadcast to requested address);
  }

  int port = JDP_DEFAULT_PORT;
@@ -310,7 +312,7 @@
 try {
port = Integer.parseInt(discoveryPort);
 } catch (NumberFormatException e) {
- throw new AgentConfigurationError(Couldn't parse JDP port 
argument);
+ throw new AgentConfigurationError(AGENT_EXCEPTION, Couldn't 
parse JDP port argument);
 }
  }

@@ -330,12 +332,7 @@

  String instanceName = 
props.getProperty(com.sun.management.jdp.name);

-try{
-   JdpController.startDiscoveryService(address, port, 
instanceName, jmxUrlStr);
-}
-catch(JdpException e){
-throw new AgentConfigurationError(Couldn't start JDP 
service, e);
-}
+JdpController.startDiscoveryService(address, port, instanceName, 
jmxUrlStr);
  }
  }

---


I want also to contribute it.
Should I merge it to JDK-8057556? Or should I file to JBS as new issue?


Thanks,

Yasumasa


(2014/09/05 19:28), Yasumasa Suenaga wrote:
 Hi Peter,

 I fixed it and created new webrev.
 http://cr.openjdk.java.net/~ysuenaga/JDK-8057556/webrev.1/

 Could you review it again?


 Thanks,

 Yasumasa


 (2014/09/05 17:20), Peter Allwin wrote:
 Looks like only the first Interface will be considered if no srcAddress is 
 provided (succeeded will be false and we will throw to exit the while loop). 
 Is this intended?

 Thanks!
 /peter

 On 4 sep 2014, at 17:59, Yasumasa Suenaga yasue...@gmail.com wrote:

 Hi all,

 Thank you so much, Dmitry!

 I've created webrev for it.
 http://cr.openjdk.java.net/~ysuenaga/JDK-8057556/webrev.0/

 Please review.


 Thanks,

 Yasumasa


 (2014/09/04 21:26), Dmitry Samersoff wrote:
 Yasumasa,

 The CR number is JDK-8057556

 I'll care about it's integration.

 -Dmitry

 On 2014-09-02 18:52, Yasumasa Suenaga wrote:
 Hi all,

 I'm trying to use JDP on my Fedora20 machine.
 My machine

Re: JDP broadcaster issue

[Dmitry Samersoff]

Yasumasa,

The CR number is JDK-8057556

I'll care about it's integration.

-Dmitry

On 2014-09-02 18:52, Yasumasa Suenaga wrote:
 Hi all,
 
 I'm trying to use JDP on my Fedora20 machine.
 My machine has two NICs and only one NIC is up.
 
 I passed system properties as below, however JDP broadcaster
 thread was not started:
 
   -Dcom.sun.management.jmxremote.port=7091
   -Dcom.sun.management.jmxremote.authenticate=false
   -Dcom.sun.management.jmxremote.ssl=false
   -Dcom.sun.management.jmxremote.autodiscovery=true
   -Dcom.sun.management.jdp.name=TEST
 
 I checked exceptions with jdb, SocketException was occurred in
 JDPControllerRunner#run(), and it was caused by another NIC
 is down.
 
 Currently, DiagramChannel which is used in JDPBroadcaster
 tries to send JDP packet through all UP NICs.
 However, NIC which is controlled by NetworkManager seems to
 be still UP when ifdown command is executed.
 (It seems to be removed IP address from NIC only.)
 
 
 This problem may be Fedora, however I think it should be
 improved in JDK.
 I've created a patch as below, and it works fine in my environment.
 (jdk9/dev/jdk)
 
 If this patch may be accepted, I will file this to JBS.
 
 
 diff -r 68a6bb51cb26 
 src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 --- 
 a/src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 Mon Sep 01 13:33:28 2014 +0200
 +++ 
 b/src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 Tue Sep 02 23:25:50 2014 +0900
 @@ -35,6 +35,7 @@
  import java.nio.ByteBuffer;
  import java.nio.channels.DatagramChannel;
  import java.nio.channels.UnsupportedAddressTypeException;
 +import java.util.Enumeration;
  
  /**
   * JdpBroadcaster is responsible for sending pre-built JDP packet across a 
 Net
 @@ -79,6 +80,15 @@
  if (srcAddress != null) {
  // User requests particular interface to bind to
  NetworkInterface interf = 
 NetworkInterface.getByInetAddress(srcAddress);
 +
 +if (interf == null) {
 +throw new JdpException(Unable to get network interface for 
  + srcAddress.toString());
 +}
 +
 +if (!interf.isUp() || !interf.supportsMulticast()) {
 +throw new JdpException(interf.getName() +  does not support 
 multicast.);
 +}
 +
  try {
  channel.bind(new InetSocketAddress(srcAddress, 0));
  } catch (UnsupportedAddressTypeException ex) {
 @@ -86,6 +96,23 @@
  }
  channel.setOption(StandardSocketOptions.IP_MULTICAST_IF, interf);
  }
 +else {
 +EnumerationNetworkInterface nics = 
 NetworkInterface.getNetworkInterfaces();
 +while (nics.hasMoreElements()) {
 +NetworkInterface nic = nics.nextElement();
 +
 +if (nic.isUp()  nic.supportsMulticast()) {
 +try {
 +
 channel.setOption(StandardSocketOptions.IP_MULTICAST_IF, nic);
 +} catch (IOException ex) {
 +System.err.println(WARNING: JDP broadcaster cannot 
 use  + nic.getName() + :  + ex.getMessage());
 +}
 +}
 +
 +}
 +
 +}
 +
  }
  
  /**
 
 
 
 Thanks,
 
 Yasumasa
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8057556: JDP should better handle non-active interfaces

[Dmitry Samersoff]

Looks good for me!

On 2014-09-04 19:59, Yasumasa Suenaga wrote:
 Hi all,
 
 Thank you so much, Dmitry!
 
 I've created webrev for it.
 http://cr.openjdk.java.net/~ysuenaga/JDK-8057556/webrev.0/
 
 Please review.
 
 
 Thanks,
 
 Yasumasa
 
 
 (2014/09/04 21:26), Dmitry Samersoff wrote:
 Yasumasa,

 The CR number is JDK-8057556

 I'll care about it's integration.

 -Dmitry

 On 2014-09-02 18:52, Yasumasa Suenaga wrote:
 Hi all,

 I'm trying to use JDP on my Fedora20 machine.
 My machine has two NICs and only one NIC is up.

 I passed system properties as below, however JDP broadcaster
 thread was not started:

-Dcom.sun.management.jmxremote.port=7091
-Dcom.sun.management.jmxremote.authenticate=false
-Dcom.sun.management.jmxremote.ssl=false
-Dcom.sun.management.jmxremote.autodiscovery=true
-Dcom.sun.management.jdp.name=TEST

 I checked exceptions with jdb, SocketException was occurred in
 JDPControllerRunner#run(), and it was caused by another NIC
 is down.

 Currently, DiagramChannel which is used in JDPBroadcaster
 tries to send JDP packet through all UP NICs.
 However, NIC which is controlled by NetworkManager seems to
 be still UP when ifdown command is executed.
 (It seems to be removed IP address from NIC only.)


 This problem may be Fedora, however I think it should be
 improved in JDK.
 I've created a patch as below, and it works fine in my environment.
 (jdk9/dev/jdk)

 If this patch may be accepted, I will file this to JBS.

 
 diff -r 68a6bb51cb26 
 src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 --- 
 a/src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java  
 Mon Sep 01 13:33:28 2014 +0200
 +++ 
 b/src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java  
 Tue Sep 02 23:25:50 2014 +0900
 @@ -35,6 +35,7 @@
   import java.nio.ByteBuffer;
   import java.nio.channels.DatagramChannel;
   import java.nio.channels.UnsupportedAddressTypeException;
 +import java.util.Enumeration;
   
   /**
* JdpBroadcaster is responsible for sending pre-built JDP packet across 
 a Net
 @@ -79,6 +80,15 @@
   if (srcAddress != null) {
   // User requests particular interface to bind to
   NetworkInterface interf = 
 NetworkInterface.getByInetAddress(srcAddress);
 +
 +if (interf == null) {
 +throw new JdpException(Unable to get network interface 
 for  + srcAddress.toString());
 +}
 +
 +if (!interf.isUp() || !interf.supportsMulticast()) {
 +throw new JdpException(interf.getName() +  does not 
 support multicast.);
 +}
 +
   try {
   channel.bind(new InetSocketAddress(srcAddress, 0));
   } catch (UnsupportedAddressTypeException ex) {
 @@ -86,6 +96,23 @@
   }
   channel.setOption(StandardSocketOptions.IP_MULTICAST_IF, 
 interf);
   }
 +else {
 +EnumerationNetworkInterface nics = 
 NetworkInterface.getNetworkInterfaces();
 +while (nics.hasMoreElements()) {
 +NetworkInterface nic = nics.nextElement();
 +
 +if (nic.isUp()  nic.supportsMulticast()) {
 +try {
 +
 channel.setOption(StandardSocketOptions.IP_MULTICAST_IF, nic);
 +} catch (IOException ex) {
 +System.err.println(WARNING: JDP broadcaster 
 cannot use  + nic.getName() + :  + ex.getMessage());
 +}
 +}
 +
 +}
 +
 +}
 +
   }
   
   /**
 


 Thanks,

 Yasumasa





-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDP broadcaster issue

[Dmitry Samersoff]

Yasumasa,

Thank you for the patch. I'll create a bug for it.

-Dmitry

On 2014-09-02 18:52, Yasumasa Suenaga wrote:
 Hi all,
 
 I'm trying to use JDP on my Fedora20 machine.
 My machine has two NICs and only one NIC is up.
 
 I passed system properties as below, however JDP broadcaster
 thread was not started:
 
   -Dcom.sun.management.jmxremote.port=7091
   -Dcom.sun.management.jmxremote.authenticate=false
   -Dcom.sun.management.jmxremote.ssl=false
   -Dcom.sun.management.jmxremote.autodiscovery=true
   -Dcom.sun.management.jdp.name=TEST
 
 I checked exceptions with jdb, SocketException was occurred in
 JDPControllerRunner#run(), and it was caused by another NIC
 is down.
 
 Currently, DiagramChannel which is used in JDPBroadcaster
 tries to send JDP packet through all UP NICs.
 However, NIC which is controlled by NetworkManager seems to
 be still UP when ifdown command is executed.
 (It seems to be removed IP address from NIC only.)
 
 
 This problem may be Fedora, however I think it should be
 improved in JDK.
 I've created a patch as below, and it works fine in my environment.
 (jdk9/dev/jdk)
 
 If this patch may be accepted, I will file this to JBS.
 
 
 diff -r 68a6bb51cb26 
 src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 --- 
 a/src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 Mon Sep 01 13:33:28 2014 +0200
 +++ 
 b/src/java.management/share/classes/sun/management/jdp/JdpBroadcaster.java
 Tue Sep 02 23:25:50 2014 +0900
 @@ -35,6 +35,7 @@
  import java.nio.ByteBuffer;
  import java.nio.channels.DatagramChannel;
  import java.nio.channels.UnsupportedAddressTypeException;
 +import java.util.Enumeration;
  
  /**
   * JdpBroadcaster is responsible for sending pre-built JDP packet across a 
 Net
 @@ -79,6 +80,15 @@
  if (srcAddress != null) {
  // User requests particular interface to bind to
  NetworkInterface interf = 
 NetworkInterface.getByInetAddress(srcAddress);
 +
 +if (interf == null) {
 +throw new JdpException(Unable to get network interface for 
  + srcAddress.toString());
 +}
 +
 +if (!interf.isUp() || !interf.supportsMulticast()) {
 +throw new JdpException(interf.getName() +  does not support 
 multicast.);
 +}
 +
  try {
  channel.bind(new InetSocketAddress(srcAddress, 0));
  } catch (UnsupportedAddressTypeException ex) {
 @@ -86,6 +96,23 @@
  }
  channel.setOption(StandardSocketOptions.IP_MULTICAST_IF, interf);
  }
 +else {
 +EnumerationNetworkInterface nics = 
 NetworkInterface.getNetworkInterfaces();
 +while (nics.hasMoreElements()) {
 +NetworkInterface nic = nics.nextElement();
 +
 +if (nic.isUp()  nic.supportsMulticast()) {
 +try {
 +
 channel.setOption(StandardSocketOptions.IP_MULTICAST_IF, nic);
 +} catch (IOException ex) {
 +System.err.println(WARNING: JDP broadcaster cannot 
 use  + nic.getName() + :  + ex.getMessage());
 +}
 +}
 +
 +}
 +
 +}
 +
  }
  
  /**
 
 
 
 Thanks,
 
 Yasumasa
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDK-8036003: Add variable not to separate debug information.

[Dmitry Samersoff]

Magnus,

Not, we are not doing it now.

But we should consider doing it in a future and therefore keep it in
mind when designing option to choose debug symbol mode.

-Dmitry

On 2014-03-24 15:18, Magnus Ihse Bursie wrote:
 On 2014-03-21 10:36, Dmitry Samersoff wrote:

 (c) Compression mode:

 1. none
 2. per-section compression, SHF_GNU_COMPRESSED [1]
 3. zip entire file

 
 Is 2 something we're doing? I couldn't find any references to it in the
 code. Or is it something we're planning to do?
 
 /Magnus


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S): 8038233 : Fix unsafe strcpy in Java_sun_tools_attach_{Aix, Bsd, Linux}VirtualMachine_connect()

[Dmitry Samersoff]

Volker,

I think we should check the length of passed filename and
throw an exception if filename is too long.

Otherwise we can end up opening wrong file with possibly not expected
permissions.

-Dmitry

On 2014-03-27 22:08, Volker Simonis wrote:
 Hi,
 
 a security audit for the PPC64/AIX port revealed an unsecure useage of
 'strcpy' in Java_sun_tools_attach_AixVirtualMachine_connect(). Because
 the same coding is also used in the Linux and BSD implementations, the
 following change fixes them all together:
 
 http://cr.openjdk.java.net/~simonis/webrevs/8038233/
 https://bugs.openjdk.java.net/browse/JDK-8038233
 
 Compiled and tested (with the com/sun/jdi, com/sun/tools/attach,
 com/sun/management and sun/management JTreg tests) on Linux, MacOS X
 and AIX.
 
 Please notice that this fix is also intended for backporting tu 8u.
 
 Thank you and best regards,
 Volker
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDK-8036003: Add variable not to separate debug information.

[Dmitry Samersoff]

David,

In practice, we don't have to much to keep internally.

There are no reason to copy out some of .debug_* sections but keep other
ones.

So we have a matrix:

(a) Strip mode:

1. full
2. keep symbols
3. keep symbols and .debug_*


(b) Copy mode:

1. Copy all to ext file
2. Copy none to ext file


(c) Compression mode:

1. none
2. per-section compression, SHF_GNU_COMPRESSED [1]
3. zip entire file

Where
  *a2 + b2 + c2* have perfect sense,
  *a1 + b1 + c3* have no sense etc.


-Dmitry


Refs:
1.
https://sourceware.org/bugzilla/show_bug.cgi?id=11819
http://gcc.gnu.org/ml/gcc-patches/2013-04/msg01780.html


-Dmitry

On 2014-03-21 12:57, David Holmes wrote:
 On 21/03/2014 6:14 PM, Magnus Ihse Bursie wrote:

 I don't think this quite works as there are other variations not
 captured here. Rather than zipped it should just be external.
 Whether the debuginfo files are zipped or not is then an additional
 build time option. Additionally we still have to be able to control
 the degree of stripping that is carried out. It doesn't make sense to
 me to try and enumerate all possible combinations as top-level
 configure options.

 Further, as Dan was saying, this doesn't capture the overlap between
 internal and external because we still leave some symbols internal
 even when creating the debuginfo file.

 So I don't think this proposed categorization works. We still have
 three aspects:
 - generating symbols into the object files
 - stripping symbols from the final binaries
 - saving symbols into an external form

 Each of which can potentally be varied (of course if you don't
 generate symbols in the obj files stripping and saving are non issues).

 But they are not independent on each other!

 Unless you generate debug symbols, you can't strip them, nor save them
 elsewhere. So generating debug symbols (your item #1) is a prerequisite
 for the rest of your items.
 
 Yes I just said that. :)
 
 And while, technically, you can save symbols externally, and at the same
 time keep them in the binary, that does not make sense. So, in a
 practical sense, you are going to do #2 if you are going to do #3.
 
 But you can vary what is kept internally independent of what is made
 external.
 
 And you can't zip external symbols unless you create a non-zipped
 version. And if you zip them, it does not make sense to keep the
 non-zipped version.
 
 zip vs non-zip is just an issue of disk space. It is not a fundamental
 configuration choice, just a variation on how external symbols are
 packaged.
 
 And yes, we do not strip all debug information when creating external
 debug info. But there seems to be no real use case (not even for
 IcedTea, as it turned out) to want a completely stripped binary, I don't
 think that's worth discussing much. That's just part of how the external
 debuginfo scheme works.
 
 Umm we fully strip all our binaries in embedded.
 
 Can you give a more concrete example of the variations of your aspects
 that you think my suggested solution would not capture?
 
 I think I already have. There aren't tree simple choices here, there are
 three aspects, each of which could have different variants.
 
 If I could draw a flow chart easily I would but basically if you
 generate debug symbols you can then select what symbols are kept
 internally (the strip policy) and what are put externally (objcopy
 options), then for the external symbol file you can choose zipped or
 unzipped.
 
 Multiple inter-connected choices, not just three (or four if you add
 zipped)
 
 David
 
 /Magnus


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.

Re: JDK-8036003: Add variable not to separate debug information.

[Dmitry Samersoff]

David,

My point was that we don't need in fine grained selection of elf
sections on strip - three options are enough.

 Why does full strip + copy-all + zip make no sense? It is exactly what
 we do with embedded builds. (Naturally you have to copy before you
 strip)

Sorry! it should be: full strip + copy-none + zip

-Dmitry

On 2014-03-21 15:13, David Holmes wrote:
 On 21/03/2014 7:36 PM, Dmitry Samersoff wrote:
 David,

 In practice, we don't have to much to keep internally.

 There are no reason to copy out some of .debug_* sections but keep other
 ones.
 
 I'm not familiar with exactly what the different strip options do but
 the point is this is covered by the strip-policy.
 
 So we have a matrix:

 (a) Strip mode:

 1. full
 2. keep symbols
 3. keep symbols and .debug_*


 (b) Copy mode:

 1. Copy all to ext file
 2. Copy none to ext file


 (c) Compression mode:

 1. none
 2. per-section compression, SHF_GNU_COMPRESSED [1]
 3. zip entire file

 Where
*a2 + b2 + c2* have perfect sense,
 
 So now your compression mode may apply to either the external file or
 the original? That's even more permutations.
 
*a1 + b1 + c3* have no sense etc.
 
 Why does full strip + copy-all + zip make no sense? It is exactly what
 we do with embedded builds. (Naturally you have to copy before you strip)
 
 David
 -
 

 -Dmitry


 Refs:
 1.
 https://sourceware.org/bugzilla/show_bug.cgi?id=11819
 http://gcc.gnu.org/ml/gcc-patches/2013-04/msg01780.html


 -Dmitry

 On 2014-03-21 12:57, David Holmes wrote:
 On 21/03/2014 6:14 PM, Magnus Ihse Bursie wrote:

 I don't think this quite works as there are other variations not
 captured here. Rather than zipped it should just be external.
 Whether the debuginfo files are zipped or not is then an additional
 build time option. Additionally we still have to be able to control
 the degree of stripping th
 *Subject:* Fwd: Re: Compute sizes of classes loat is carried out. It doesn't 
 make sense to
 me to try and enumerate all possible combinations as top-level
 configure options.

 Further, as Dan was saying, this doesn't capture the overlap between
 internal and external because we still leave some symbols internal
 even when creating the debuginfo file.

 So I don't think this proposed categorization works. We still have
 three aspects:
 - generating symbols into the object files
 - stripping symbols from the final binaries
 - saving symbols into an external form

 Each of which can potentally be varied (of course if you don't
 generate symbols in the obj files stripping and saving are non
 issues).

 But they are not independent on each other!

 Unless you generate debug symbols, you can't strip them, nor save them
 elsewhere. So generating debug symbols (your item #1) is a prerequisite
 for the rest of your items.

 Yes I just said that. :)

 And while, technically, you can save symbols externally, and at the
 same
 time keep them in the binary, that does not make sense. So, in a
 practical sense, you are going to do #2 if you are going to do #3.

 But you can vary what is kept internally independent of what is made
 external.

 And you can't zip external symbols unless you create a non-zipped
 version. And if you zip them, it does not make sense to keep the
 non-zipped version.

 zip vs non-zip is just an issue of disk space. It is not a fundamental
 configuration choice, just a variation on how external symbols are
 packaged.

 And yes, we do not strip all debug information when creating external
 debug info. But there seems to be no real use case (not even for
 IcedTea, as it turned out) to want a completely stripped binary, I
 don't
 think that's worth discussing much. That's just part of how the
 external
 debuginfo scheme works.

 Umm we fully strip all our binaries in embedded.

 Can you give a more concrete example of the variations of your
 aspects
 that you think my suggested solution would not capture?

 I think I already have. There aren't tree simple choices here, there are
 three aspects, each of which could have different variants.

 If I could draw a flow chart easily I would but basically if you
 generate debug symbols you can then select what symbols are kept
 internally (the strip policy) and what are put externally (objcopy
 options), then for the external symbol file you can choose zipped or
 unzipped.

 Multiple inter-connected choices, not just three (or four if you add
 zipped)

 David

 /Magnus




-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.

Re: RFR(S): JDK-8033911 Simplify instrumentation of FileInputStream and RandomAccessFile

[Dmitry Samersoff]

Staffan,

As far as you touching this.

Is it possible to change all native methods in these two classes to have
0 at the end of name?

i.e. readBytes = readBytes0

it's pure cosmetic, but fairly simplify core dump reading and later
grep-ing.

-Dmitry

On 2014-02-07 14:46, Staffan Larsen wrote:
 A few of the public read and write methods in FileInputStream and 
 RandomAccessFile are declared native. This means that it is hard to 
 instrument them using byte code instrumentation. Changing the public methods 
 to be to non-native and instead calling private native methods simplifies 
 instrumentation. 
 
 webrev: http://cr.openjdk.java.net/~sla/8033911/webrev.00/
 bug: https://bugs.openjdk.java.net/browse/JDK-8033911
 
 Thanks,
 /Staffan
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S): JDK-8033911 Simplify instrumentation of FileInputStream and RandomAccessFile

[Dmitry Samersoff]

Staffan,

OK! Looks good for me.

-Dmitry

On 2014-02-07 15:28, Staffan Larsen wrote:
 I would prefer that to be a different change.
 
 Thanks,
 /Staffan
 
 On 7 feb 2014, at 12:07, Dmitry Samersoff dmitry.samers...@oracle.com wrote:
 
 Staffan,

 As far as you touching this.

 Is it possible to change all native methods in these two classes to have
 0 at the end of name?

 i.e. readBytes = readBytes0

 it's pure cosmetic, but fairly simplify core dump reading and later
 grep-ing.

 -Dmitry

 On 2014-02-07 14:46, Staffan Larsen wrote:
 A few of the public read and write methods in FileInputStream and 
 RandomAccessFile are declared native. This means that it is hard to 
 instrument them using byte code instrumentation. Changing the public 
 methods to be to non-native and instead calling private native methods 
 simplifies instrumentation. 

 webrev: http://cr.openjdk.java.net/~sla/8033911/webrev.00/
 bug: https://bugs.openjdk.java.net/browse/JDK-8033911

 Thanks,
 /Staffan



 -- 
 Dmitry Samersoff
 Oracle Java development team, Saint Petersburg, Russia
 * I would love to change the world, but they won't give me the sources.
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8033917 Keep track of file paths in file streams and channels for instrumentation purposes

[Dmitry Samersoff]

Staffan,

FileInputStream.java

55:  It's better to initialize path with null.
134: It's better to assign name at one of first lines, in this case we
will be able to retrieve file name ever if open fails for some reason.
171: It's not necessary

(the same is applicable to other files)

I'm a bit scared changing signature of public methods of FileChannelImpl
but if Alan says it's OK - lets go with it.

-Dmitry


On 2014-02-07 16:07, Staffan Larsen wrote:
 Instrumentation agents that want to instrument
 FileInputStream/FileOutputStream to see which files are being
 accessed do not currently have access to the file system path of the
 stream. This is because the path is never stored in the stream class,
 only the file descriptor is. (This is also true for RandomAccessFile
 and FileChannel).
 
 An agent could instrument the respective constructors to store the
 path. The problem is where to store it. To add a field, the
 instrumentation agent needs to change the schema of the class. This
 is not possible at runtime but can be done at class-loading time.
 However for a j.l.instrument agent these classes are already defined
 when the agent is first called. For a native JVMTI agent the problem
 becomes parsing and modifying byte codes in a native agent which is
 error prone and requires a lot of code to maintain.
 
 If instead the stream classes were modified to store a reference to
 the path, it would be readily available for agents at a minimum of
 cost to the libraries. This is what this patch does. FileInputStream,
 FileOutputStream, RandomAccessFile and FileChannelImpl are modified
 to record the path they operate on in a private field. There are no
 accessors added to retrieve the path - it is purely stored for
 instrumentation purposes. The path is intentionally not resolved to
 be an absolute path since that would potentially add unwanted
 overhead. If a stream is created from a file descriptor, no path will
 be stored.
 
 The overhead for this path will be keeping the path String alive for
 a longer period of time. I hope this will not cause any problems.
 
 A consumer of this feature will be Java Flight Recorder, but the
 implementation is usable by other agents as well.
 
 webrev: http://cr.openjdk.java.net/~sla/8033917/webrev.00/ bug:
 https://bugs.openjdk.java.net/browse/JDK-8033917
 
 Thanks, /Staffan
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8033917 Keep track of file paths in file streams and channels for instrumentation purposes

[Dmitry Samersoff]

On 2014-02-07 19:32, Chris Hegarty wrote:
 On 07/02/14 15:24, Dmitry Samersoff wrote:
 Staffan,

 FileInputStream.java

 55:  It's better to initialize path with null.
 
 I'm afraid I disagree with this. The default value is already null, why
 set it to null again? I see this pattern all over the code, but it seems
 completely redundant to me.

Yes, It's NOOP but it makes readers and variety of security tools happy.

I will not press for it, but as far as rest of the code

(e.g. private FileChannel channel = null; )

uses this pattern and initialize variables explicitly, I think it's good
to initialize this variable as well.

-Dmitry


 
 -Chris.
 
 134: It's better to assign name at one of first lines, in this case we
 will be able to retrieve file name ever if open fails for some reason.
 171: It's not necessary

 (the same is applicable to other files)

 I'm a bit scared changing signature of public methods of FileChannelImpl
 but if Alan says it's OK - lets go with it.

 -Dmitry


 On 2014-02-07 16:07, Staffan Larsen wrote:
 Instrumentation agents that want to instrument
 FileInputStream/FileOutputStream to see which files are being
 accessed do not currently have access to the file system path of the
 stream. This is because the path is never stored in the stream class,
 only the file descriptor is. (This is also true for RandomAccessFile
 and FileChannel).

 An agent could instrument the respective constructors to store the
 path. The problem is where to store it. To add a field, the
 instrumentation agent needs to change the schema of the class. This
 is not possible at runtime but can be done at class-loading time.
 However for a j.l.instrument agent these classes are already defined
 when the agent is first called. For a native JVMTI agent the problem
 becomes parsing and modifying byte codes in a native agent which is
 error prone and requires a lot of code to maintain.

 If instead the stream classes were modified to store a reference to
 the path, it would be readily available for agents at a minimum of
 cost to the libraries. This is what this patch does. FileInputStream,
 FileOutputStream, RandomAccessFile and FileChannelImpl are modified
 to record the path they operate on in a private field. There are no
 accessors added to retrieve the path - it is purely stored for
 instrumentation purposes. The path is intentionally not resolved to
 be an absolute path since that would potentially add unwanted
 overhead. If a stream is created from a file descriptor, no path will
 be stored.

 The overhead for this path will be keeping the path String alive for
 a longer period of time. I hope this will not cause any problems.

 A consumer of this feature will be Java Flight Recorder, but the
 implementation is usable by other agents as well.

 webrev: http://cr.openjdk.java.net/~sla/8033917/webrev.00/ bug:
 https://bugs.openjdk.java.net/browse/JDK-8033917

 Thanks, /Staffan





-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR: JDK-8033917 Keep track of file paths in file streams and channels for instrumentation purposes

[Dmitry Samersoff]

Staffan,

OK!

-Dmitry

On 2014-02-07 19:49, Staffan Larsen wrote:
 
 On 7 feb 2014, at 16:24, Dmitry Samersoff dmitry.samers...@oracle.com wrote:
 
 Staffan,

 FileInputStream.java

 55:  It's better to initialize path with null.
 
 I agree with Chris here. The value should be explicitly initialized by all 
 constructors.
 
 134: It's better to assign name at one of first lines, in this case we
 will be able to retrieve file name ever if open fails for some reason.
 
 This is the constructor. If anything fails it will throw and exception, and 
 there won’t be an object to look at.
 
 171: It's not necessary
 
 All constructors must initialize the value. 
 
 Thanks,
 /Staffan
 

 (the same is applicable to other files)

 I'm a bit scared changing signature of public methods of FileChannelImpl
 but if Alan says it's OK - lets go with it.

 -Dmitry


 On 2014-02-07 16:07, Staffan Larsen wrote:
 Instrumentation agents that want to instrument
 FileInputStream/FileOutputStream to see which files are being
 accessed do not currently have access to the file system path of the
 stream. This is because the path is never stored in the stream class,
 only the file descriptor is. (This is also true for RandomAccessFile
 and FileChannel).

 An agent could instrument the respective constructors to store the
 path. The problem is where to store it. To add a field, the
 instrumentation agent needs to change the schema of the class. This
 is not possible at runtime but can be done at class-loading time.
 However for a j.l.instrument agent these classes are already defined
 when the agent is first called. For a native JVMTI agent the problem
 becomes parsing and modifying byte codes in a native agent which is
 error prone and requires a lot of code to maintain.

 If instead the stream classes were modified to store a reference to
 the path, it would be readily available for agents at a minimum of
 cost to the libraries. This is what this patch does. FileInputStream,
 FileOutputStream, RandomAccessFile and FileChannelImpl are modified
 to record the path they operate on in a private field. There are no
 accessors added to retrieve the path - it is purely stored for
 instrumentation purposes. The path is intentionally not resolved to
 be an absolute path since that would potentially add unwanted
 overhead. If a stream is created from a file descriptor, no path will
 be stored.

 The overhead for this path will be keeping the path String alive for
 a longer period of time. I hope this will not cause any problems.

 A consumer of this feature will be Java Flight Recorder, but the
 implementation is usable by other agents as well.

 webrev: http://cr.openjdk.java.net/~sla/8033917/webrev.00/ bug:
 https://bugs.openjdk.java.net/browse/JDK-8033917

 Thanks, /Staffan



 -- 
 Dmitry Samersoff
 Oracle Java development team, Saint Petersburg, Russia
 * I would love to change the world, but they won't give me the sources.
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

hg: jdk8/tl/jdk: 8024071: In ManagementAgent.start it should be possible to set the jdp.name parameter.

[dmitry . samersoff]

Changeset: 392acefef659
Author:dsamersoff
Date:  2013-10-19 20:59 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/392acefef659

8024071: In ManagementAgent.start it should be possible to set the jdp.name 
parameter.
Summary: Pass one more property from Agent to JdpController
Reviewed-by: jbachorik, sla

! src/share/classes/sun/management/Agent.java
! test/sun/management/jdp/JdpTest.sh
! test/sun/management/jmxremote/startstop/JMXStartStopTest.sh

hg: jdk8/tl/jdk: 8004213: JDP packet needs pid, broadcast interval and rmi server hostname fields

[dmitry . samersoff]

Changeset: 88436832cfd0
Author:dsamersoff
Date:  2013-10-19 00:05 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/88436832cfd0

8004213: JDP packet needs pid, broadcast interval and rmi server hostname fields
Summary: Add some extra fileds to jdp packet
Reviewed-by: allwin, sla, hirt

! src/share/classes/sun/management/jdp/JdpController.java
! src/share/classes/sun/management/jdp/JdpJmxPacket.java
! test/sun/management/jdp/JdpClient.java
! test/sun/management/jdp/JdpDoSomething.java
! test/sun/management/jdp/JdpTest.sh

Re: JDK 8 RFR 8026806: Incomplete test of getaddrinfo() return value could lead to incorrect exception for Windows Inet 6

[Dmitry Samersoff]

Brian,

Looks good for me (not a reviewer).

-Dmitry

On 2013-10-17 20:46, Brian Burkhalter wrote:
 Continuing the discussion from 
 http://mail.openjdk.java.net/pipermail/net-dev/2013-October/007574.html under 
 new issue ID:
 
 Issue:https://bugs.openjdk.java.net/browse/JDK-8026806
 Webrev:   http://cr.openjdk.java.net/~bpb/8026806/webrev/
 
 Thanks,
 
 Brian
 
 On Oct 17, 2013, at 8:06 AM, Brian Burkhalter wrote:
 

 On Oct 17, 2013, at 5:37 AM, Alan Bateman wrote:

 On 17/10/2013 00:21, Brian Burkhalter wrote:
 Dmitry,

 I think you are correct: that slipped by both me and the reviewers. I have 
 reopened the issue and posted an amendment to the original webrev here:

 http://cr.openjdk.java.net/~bpb/8010371/webrev.4-amendment/

 I've restored the bug fields and I assume you'll create a new bug for the 
 follow-up. Sorry this was missed in the original review (probably because 
 it went through so many iterations).

 -Alan.

 Yes I will create a new one, thanks.

 Brian
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[Dmitry Samersoff]

Alan,

getaddrinfo could return EAI_AGAIN[1]

But recomended way is to just check return value of getaddrinfo and then
use WSA functions especially, FormatMessage[2] instead of gai_strerror.

Also we may consider rewriting this code using GetAddrInfoW sometimes in
a future to support IDN.

PS:
Brian, please notice mixing WSA and EAI at ll. 137.


[1]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms738520(v=vs.85).aspx

[2]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms679351(v=vs.85).aspx


On 2013-10-10 17:52, Alan Bateman wrote:
 On 09/10/2013 19:16, Brian Burkhalter wrote:
 :
 I have created a simple implementation for option B:

 http://cr.openjdk.java.net/~bpb/8010371/webrev.3/

 I should note that the Unix Inet6 implementation was already using the call 
 ThrowUnknownHostExceptionWithGaiError() to generate the UHE so in this case 
 the message should already have been useful. This call formats the UHE 
 message such as would:

 sprintf(error_message, %s: %s, hostname, gai_strerror(error))

 I changed the Unix Inet4 implementation to do this as well instead of 
 calling JNU_ThrowByName().
 Thanks for persevering with this, we are on the right road now.
 
 I just looked at JDK-8010371 and the OS field is set to linux. I
 don't know if this is right but as IPv6 is usually enabled by default
 these days then I have to guess that the person that submitted the bug
 has IPv6 disabled or is running with -Djava.net.preferIPv4Stack=true,
 otherwise it would be a non-issue (as you have discovered).
 
 The other thing about your observation is that
 ThrowUnknownHostExceptionWithGaiError is creating the UHE with the both
 the hostname and the error message. This resolves to the concern in one
 or two of the mails that the UHE names the exception message host and
 that someone might assume that the exception detail is the hostname.
 
 So thumbs up on the update to src/solaris/native/java/net/Inet4Address.c.
 

 For Windows I added a similar NET_ ThrowUnknownHostExceptionWithGaiError and 
 modified Inet{4,6} to mimic the Unix case. Note that the Windows code has 
 not yet been compiled pending comments.

 Inet4AddressImpl.lookupAllHostAddr uses gethostbyname and when I look at
 the MSDN documentation then I don't see EAI_AGAIN as possible error. It
 does list WSATRY_AGAIN and I'm wondering if they have the same value and
 whether the WSA* error is only returned by WSAGetLastError?
 
 However I think we have a problem with using gai_strerror here as the
 MSDN documentation says that it is not thread safe. This means we may
 have to look for a WSA equivalent or maybe drop the Windows part of the
 fix (using a mutex of some synchronization might be possible but the
 scope would require research and of course it wouldn't work with 3rd
 party native code that is also using it).
 
 -Alan
 
 
 
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[Dmitry Samersoff]

Chris,

On 2013-10-03 14:10, Chris Hegarty wrote:
 Thanks Dmitry,
 
 I think we have agreement that the cause of the UHE should contain an
 Exception containing the gai_strerror string message. I can live without
 adding retry logic.

I'm ok with that.

-Dmitry


 
 -Chris.
 
 On 10/03/2013 10:44 AM, Dmitry Samersoff wrote:
 Chris,

 On my opinion, it's better to just return meaningful exception to
 customer and let them deal with network issue (as current webrev does).

 Typical network failure requires at least couple of milliseconds to
 recover so immediate retry most probably fails with the same error.

  From the other side - cu has lots of possibility to defend against such
 failures on application level. E.g. popup a message box please check
 your wiring and try again

 In a future,

 it might be possible to add a timeout parameter to corresponding
 Java-level functions and keep trying on Java level until we get result
 or timeout, but it requires much more work and probably out of scope of
 this CR.

 -Dmitry


 On 2013-10-03 13:11, Chris Hegarty wrote:
 On 10/02/2013 11:18 PM, Dmitry Samersoff wrote:
 Chris,

 I'm not sure immediate native retry make sence here because tipically
 EAGAIN of getaddrinfo caused by network failure, like unreachable
 nameserver. (see my previous letter)

 OK, while not ideal ( please don't shoot! ) what do others think of
 Thread.yield() before retry. It is an attempt to allow other threads on
 the system do some work before us, therefore potentially swapping out
 our failed lookup thread until rescheduled. I'm just trying to avoid
 baking in a hardcoded sleep/wait value ( since we don't know what that
 should be ).

 The use of Thread.yield(), if acceptable, would of course most likely
 make sense to push the retry logic back up into the Java level.

 -Chris.


 -Dmitry

 On 2013-10-02 23:53, Chris Hegarty wrote:
 On 10/02/2013 08:40 PM, Brian Burkhalter wrote:
 
 So, how about this approach:

 1) If the error is EAI_AGAIN / EIA_SYSTEM+EAGAIN / WSATRY_AGAIN then
 do one immediate native retry.
 2) If the retry fails with the same error, then throw a UHE with a
 specific message or cause.

 Sounds good to me.

 Questions:

 A) Would it be better to do the retry in the Java layer, perhaps with
 a very short wait?

 native, without any wait, should be sufficient.

 B) Should the message or cause in #2 be explicitly document in the
 javadoc?

 I don't think it is necessary for this to be documented. It is more
 informational.

 -Chris.






-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(S): 7200277 [parfait] potential buffer overflow in npt/utf.c

[Dmitry Samersoff]

Staffan,

Looks good for me.

PS:
  is redundant.

-Dmitry

On 2013-09-20 13:49, Staffan Larsen wrote:
 Please review this change to avoid a buffer overflow in npt/utf.c.
 
 webrev: http://cr.openjdk.java.net/~sla/7200277/webrev.00/
 bug: https://bugs.openjdk.java.net/browse/JDK-7200277
 
 Thanks,
 /Staffan
 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the source code.

hg: jdk8/tl/jdk: 8011038: sourceObj validation during desereliazation of RelationNotification should be relaxed

[dmitry . samersoff]

Changeset: fce446b29577
Author:dsamersoff
Date:  2013-08-06 14:04 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/fce446b29577

8011038: sourceObj validation during desereliazation of RelationNotification 
should be relaxed
Summary: sourceObj could be set to null by setSource() relax a validation of 
deserialized object.
Reviewed-by: sjiang, skoivu, dfuchs

! src/share/classes/javax/management/relation/RelationNotification.java

hg: jdk8/tl/jdk: 8014420: Default JDP address does not match the one assigned by IANA

[dmitry . samersoff]

Changeset: 7d7bfce34a79
Author:dsamersoff
Date:  2013-05-28 18:46 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7d7bfce34a79

8014420: Default JDP address does not match the one assigned by IANA
Summary: JDP protocol defaults changed to IANA assigned values
Reviewed-by: dholmes, jbachorik, hirt
Contributed-by: fwei...@redhat.com

! src/share/classes/sun/management/Agent.java
! src/share/classes/sun/management/jdp/package-info.java
! test/sun/management/jdp/JdpTest.sh

Re: RFR: Project files for Solaris Studio / NetBeans

[Dmitry Samersoff]

Jesper,

Tried to apply your patch as I'm quite interesting to have really
working NB project for jdk and hotspot.

1. Directory structure looks strange (see screenshot1)

2. netbeans doesn't work for JDK tests (see screenshot2) -

   netbeans doesn't pick files
   from test directory (JDK-only project have the same issue)

   netbeans doesn't pick changes from files in JDK tree - i.e
   if I add/change class name in JDK tree, netbeans still treated it
   as unresolved within test (JDK-only project have the same issue)

-Dmitry



On 2013-03-25 20:29, Jesper Wilhelmsson wrote:
 Hi,
 
 Sorry for cross posting, but I think this could be useful for several
 areas.
 
 I would like to add Solaris Studio / NetBeans project files for the
 entire OpenJDK project. To clarify: One project that contains the entire
 OpenJDK.
 
 
 With the new build infrastructure in JDK 8 building the entire OpenJDK
 is fairly fast and even though I personally mostly work in the HotSpot
 tree, I tend to always clone and build the entire JDK forest. I find
 this to have several benefits.
 
 Webrev: http://cr.openjdk.java.net/~jwilhelm/7074926/webrev/
 
 The configuration in this project is currently Mac only. Linux and
 Solaris configurations are also planned.
 
 The webrev is made from the jdk8/build repository which is where I think
 a change like this should go in. Let me know if you think something else.
 
 
 
 To use this project (once pushed):
 
 1. Clone your favorite repository
hg clone http://hg.openjdk.java.net/hsx/hotspot-gc
 
 2. Get the whole forest
cd hotspot-gc
sh get_source.sh
 
 3. Configure
sh configure
 
 4. Open Solaris studio / NetBeans and load the project.
The project in located in the common directory.
 
 
 Thanks,
 /Jesper


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

Re: RFR: Project files for Solaris Studio / NetBeans

[Dmitry Samersoff]

Jesper,

Tryed to apply your patch as I'm quite inetresting to have really
working NB project for jdk and hotspot.

1. Directory structure looks strange (see screenshot1)

2. netbeans doesn't work for JDK tests (see screenshot2) -

   netbeans doesn't pick files
   from test directory (JDK-only project have the same issue)

   netbeans doesn't pick changes from files in JDK tree - i.e
   if I add/change class name in JDK tree, netbeans still treated it
   as unresolved within test (JDK-only project have the same issue)

-Dmitry



On 2013-03-25 20:29, Jesper Wilhelmsson wrote:
 Hi,
 
 Sorry for cross posting, but I think this could be useful for several
 areas.
 
 I would like to add Solaris Studio / NetBeans project files for the
 entire OpenJDK project. To clarify: One project that contains the entire
 OpenJDK.
 
 
 With the new build infrastructure in JDK 8 building the entire OpenJDK
 is fairly fast and even though I personally mostly work in the HotSpot
 tree, I tend to always clone and build the entire JDK forest. I find
 this to have several benefits.
 
 Webrev: http://cr.openjdk.java.net/~jwilhelm/7074926/webrev/
 
 The configuration in this project is currently Mac only. Linux and
 Solaris configurations are also planned.
 
 The webrev is made from the jdk8/build repository which is where I think
 a change like this should go in. Let me know if you think something else.
 
 
 
 To use this project (once pushed):
 
 1. Clone your favorite repository
hg clone http://hg.openjdk.java.net/hsx/hotspot-gc
 
 2. Get the whole forest
cd hotspot-gc
sh get_source.sh
 
 3. Configure
sh configure
 
 4. Open Solaris studio / NetBeans and load the project.
The project in located in the common directory.
 
 
 Thanks,
 /Jesper


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

Re: RFR: Project files for Solaris Studio / NetBeans

[Dmitry Samersoff]

Jesper,

On 2013-03-25 22:35, Jesper Wilhelmsson wrote:
 Dmitry Samersoff skrev 25/3/13 6:11 PM:
 Jesper,

 Tryed to apply your patch as I'm quite inetresting to have really
 working NB project for jdk and hotspot.

 1. Directory structure looks strange (see screenshot1)
 
 The structure in the Projects tab shows the NetBeans project hierarchy.
 It's not related to the OpenJDK project hierarchy. The Files tab (to
 the right of the project tab) will show you the directory structure as
 it looks on disk. I think this is the tab you want to use for browsing
 the directories.

I'm using Linux and fresh netbeans copy, so fd and macosx_* projects
comes from your patch for sure.

Please take a look at it.

-Dmitry

 
 2. netbeans doesn't work for JDK tests (see screenshot2) -

 netbeans doesn't pick files
 from test directory (JDK-only project have the same issue)

 netbeans doesn't pick changes from files in JDK tree - i.e
 if I add/change class name in JDK tree, netbeans still treated it
 as unresolved within test (JDK-only project have the same issue)
 
 I have mainly looked at the hotspot code when creating the project so
 I'm not surprised to see that there are things that don't work in for
 instance the jdk. I'll look at this tomorrow.
 
 Thank you for your comments,
 /Jesper
 

 -Dmitry



 On 2013-03-25 20:29, Jesper Wilhelmsson wrote:
 Hi,

 Sorry for cross posting, but I think this could be useful for several
 areas.

 I would like to add Solaris Studio / NetBeans project files for the
 entire OpenJDK project. To clarify: One project that contains the entire
 OpenJDK.


 With the new build infrastructure in JDK 8 building the entire OpenJDK
 is fairly fast and even though I personally mostly work in the HotSpot
 tree, I tend to always clone and build the entire JDK forest. I find
 this to have several benefits.

 Webrev: http://cr.openjdk.java.net/~jwilhelm/7074926/webrev/

 The configuration in this project is currently Mac only. Linux and
 Solaris configurations are also planned.

 The webrev is made from the jdk8/build repository which is where I think
 a change like this should go in. Let me know if you think something
 else.



 To use this project (once pushed):

 1. Clone your favorite repository
 hg clone http://hg.openjdk.java.net/hsx/hotspot-gc

 2. Get the whole forest
 cd hotspot-gc
 sh get_source.sh

 3. Configure
 sh configure

 4. Open Solaris studio / NetBeans and load the project.
 The project in located in the common directory.


 Thanks,
 /Jesper




-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

Re: FAILS: test/sun/management/jdp/JdpTest.sh: test: argument expected

[Dmitry Samersoff]

Chris,

Will push it in few hours.

-Dmitry

On 2013-02-13 14:53, Chris Hegarty wrote:
 Dmitry,
 
 I have a trivial patch that resolves this issue. I would like to push it
 to jdk8/tl today before we freeze for M7, and have this failure escape
 into master.
 
 The change is to simply use conditions that are also supported by bourne
 shell .
 
 diff -r 7dcb74c3ffba test/sun/management/jdp/JdpTest.sh
 --- a/test/sun/management/jdp/JdpTest.shTue Feb 12 09:25:43 2013
 -0800
 +++ b/test/sun/management/jdp/JdpTest.shWed Feb 13 10:48:28 2013
 +
 @@ -51,7 +51,7 @@ _do_compile(){
  # sun.* packages is not included to symbol file lib/ct.sym so we have
  # to ignore it
 
 -if [ ! -f ${_testclasses} ]
 +if [ ! -d ${_testclasses} ]
  then
   mkdir -p ${_testclasses}
  fi
 @@ -319,7 +319,7 @@ rm -f ${_logname}
  rm -f ${_logname}
  rm -f ${_policyname}
 
 -if [ -e ${_testsrc}/policy.tpl ]
 +if [ -f ${_testsrc}/policy.tpl ]
  then
 
  cat ${_testsrc}/policy.tpl | \
 
 -Chris.
 
 On 12/02/2013 22:19, Dmitry Samersoff wrote:
 Chris,

 I'm not able to reproduce it locally. Do you have a link to jprt job you
 see this failure?

 -Dmitry

 On 2013-02-12 21:38, Chris Hegarty wrote:
 Dmitry,

 This test is now failing on several platforms, on jdk8 and 7u-dev

 ---

 result: Passed. Compilation successful

 #section:shell
 --messages:(3/154)--
 command: shell JdpTest.sh [--jtreg, --no-compile]
 reason: User specified action: run shell JdpTest.sh --jtreg --no-compile
 elapsed time (seconds): 0.045
 --System.out:(0/0)--
 --System.err:(1/110)--
 /export2/Users/chris/repos/jdk8/tl/master_top/jdk/test/sun/management/jdp/JdpTest.sh:

 test: argument expected
 result: Failed. Execution failed: exit code 1


 test result: Failed. Execution failed: exit code 1

 -Chris.


 On 02/12/2013 12:04 PM, dmitry.samers...@oracle.com wrote:
 Changeset: f7fb173ac833
 Author:dsamersoff
 Date:  2013-02-12 16:02 +0400
 URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833

 8007786: JDK-8002048 testcase doesn't work on Solaris
 Summary: test built in into Solaris shell doesn't have -e operator
 Reviewed-by: sla, sspitsyn

 ! test/sun/management/jdp/JdpTest.sh





-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

RR(S): 8008095.JDP-TEST3 (was Re: FAILS: test/sun/management/jdp/JdpTest.sh: test: argument expected)

[Dmitry Samersoff]

Chris,

Please take a look at the final changes:

http://cr.openjdk.java.net/~dsamersoff/8008095.JDP-TEST3/webrev.01/jdk/

I still not able to get valuable results from jprt but tried couple of
different solaris machines and hope this time the problem with this test
is gone.

I'm ready to push it right after your response.

-Dmitry

On 2013-02-13 14:53, Chris Hegarty wrote:
 Dmitry,
 
 I have a trivial patch that resolves this issue. I would like to push it
 to jdk8/tl today before we freeze for M7, and have this failure escape
 into master.
 
 The change is to simply use conditions that are also supported by bourne
 shell .
 
 diff -r 7dcb74c3ffba test/sun/management/jdp/JdpTest.sh
 --- a/test/sun/management/jdp/JdpTest.shTue Feb 12 09:25:43 2013
 -0800
 +++ b/test/sun/management/jdp/JdpTest.shWed Feb 13 10:48:28 2013
 +
 @@ -51,7 +51,7 @@ _do_compile(){
  # sun.* packages is not included to symbol file lib/ct.sym so we have
  # to ignore it
 
 -if [ ! -f ${_testclasses} ]
 +if [ ! -d ${_testclasses} ]
  then
   mkdir -p ${_testclasses}
  fi
 @@ -319,7 +319,7 @@ rm -f ${_logname}
  rm -f ${_logname}
  rm -f ${_policyname}
 
 -if [ -e ${_testsrc}/policy.tpl ]
 +if [ -f ${_testsrc}/policy.tpl ]
  then
 
  cat ${_testsrc}/policy.tpl | \
 
 -Chris.
 
 On 12/02/2013 22:19, Dmitry Samersoff wrote:
 Chris,

 I'm not able to reproduce it locally. Do you have a link to jprt job you
 see this failure?

 -Dmitry

 On 2013-02-12 21:38, Chris Hegarty wrote:
 Dmitry,

 This test is now failing on several platforms, on jdk8 and 7u-dev

 ---

 result: Passed. Compilation successful

 #section:shell
 --messages:(3/154)--
 command: shell JdpTest.sh [--jtreg, --no-compile]
 reason: User specified action: run shell JdpTest.sh --jtreg --no-compile
 elapsed time (seconds): 0.045
 --System.out:(0/0)--
 --System.err:(1/110)--
 /export2/Users/chris/repos/jdk8/tl/master_top/jdk/test/sun/management/jdp/JdpTest.sh:

 test: argument expected
 result: Failed. Execution failed: exit code 1


 test result: Failed. Execution failed: exit code 1

 -Chris.


 On 02/12/2013 12:04 PM, dmitry.samers...@oracle.com wrote:
 Changeset: f7fb173ac833
 Author:dsamersoff
 Date:  2013-02-12 16:02 +0400
 URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833

 8007786: JDK-8002048 testcase doesn't work on Solaris
 Summary: test built in into Solaris shell doesn't have -e operator
 Reviewed-by: sla, sspitsyn

 ! test/sun/management/jdp/JdpTest.sh





-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

hg: jdk8/tl/jdk: 8008095: TEST_BUG: JDK-8002048 one more testcase failure on Solaris

[dmitry . samersoff]

Changeset: 8181be9a3538
Author:dsamersoff
Date:  2013-02-13 21:06 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8181be9a3538

8008095: TEST_BUG: JDK-8002048 one more testcase failure on Solaris
Summary: fixed couple of more Solaris shell incompatibilities
Reviewed-by: chegar

! test/sun/management/jdp/JdpTest.sh

hg: jdk8/tl/jdk: 8007786: JDK-8002048 testcase doesn't work on Solaris

[dmitry . samersoff]

Changeset: f7fb173ac833
Author:dsamersoff
Date:  2013-02-12 16:02 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833

8007786: JDK-8002048 testcase doesn't work on Solaris
Summary: test built in into Solaris shell doesn't have -e operator
Reviewed-by: sla, sspitsyn

! test/sun/management/jdp/JdpTest.sh

Re: FAILS: test/sun/management/jdp/JdpTest.sh: test: argument expected

[Dmitry Samersoff]

Kelly,

Good catch but this part of test is not executed under JTReg, and this
mistake cause extra mkdir call but not a test error.

-Dmitry

On 2013-02-12 21:52, Kelly O'Hair wrote:
 +if [ ! -f ${_testclasses} ]
 needs to be
 +if [ ! -d ${_testclasses} ]
 
 
 -kto
 
 On Feb 12, 2013, at 9:38 AM, Chris Hegarty wrote:
 
 Dmitry,

 This test is now failing on several platforms, on jdk8 and 7u-dev

 ---

 result: Passed. Compilation successful

 #section:shell
 --messages:(3/154)--
 command: shell JdpTest.sh [--jtreg, --no-compile]
 reason: User specified action: run shell JdpTest.sh --jtreg --no-compile
 elapsed time (seconds): 0.045
 --System.out:(0/0)--
 --System.err:(1/110)--
 /export2/Users/chris/repos/jdk8/tl/master_top/jdk/test/sun/management/jdp/JdpTest.sh:
  test: argument expected
 result: Failed. Execution failed: exit code 1


 test result: Failed. Execution failed: exit code 1

 -Chris.


 On 02/12/2013 12:04 PM, dmitry.samers...@oracle.com wrote:
 Changeset: f7fb173ac833
 Author:dsamersoff
 Date:  2013-02-12 16:02 +0400
 URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833

 8007786: JDK-8002048 testcase doesn't work on Solaris
 Summary: test built in into Solaris shell doesn't have -e operator
 Reviewed-by: sla, sspitsyn

 ! test/sun/management/jdp/JdpTest.sh

 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

Re: FAILS: test/sun/management/jdp/JdpTest.sh: test: argument expected

[Dmitry Samersoff]

Kelly,

Do you have an idea why I didn't see this failure under jprt?

http://sthjprt.se.oracle.com/archives/2013/02/2013-02-08-192233.dsamerso.tl/

-Dmitry


On 2013-02-12 21:52, Kelly O'Hair wrote:
 +if [ ! -f ${_testclasses} ]
 needs to be
 +if [ ! -d ${_testclasses} ]
 
 
 -kto
 
 On Feb 12, 2013, at 9:38 AM, Chris Hegarty wrote:
 
 Dmitry,

 This test is now failing on several platforms, on jdk8 and 7u-dev

 ---

 result: Passed. Compilation successful

 #section:shell
 --messages:(3/154)--
 command: shell JdpTest.sh [--jtreg, --no-compile]
 reason: User specified action: run shell JdpTest.sh --jtreg --no-compile
 elapsed time (seconds): 0.045
 --System.out:(0/0)--
 --System.err:(1/110)--
 /export2/Users/chris/repos/jdk8/tl/master_top/jdk/test/sun/management/jdp/JdpTest.sh:
  test: argument expected
 result: Failed. Execution failed: exit code 1


 test result: Failed. Execution failed: exit code 1

 -Chris.


 On 02/12/2013 12:04 PM, dmitry.samers...@oracle.com wrote:
 Changeset: f7fb173ac833
 Author:dsamersoff
 Date:  2013-02-12 16:02 +0400
 URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833

 8007786: JDK-8002048 testcase doesn't work on Solaris
 Summary: test built in into Solaris shell doesn't have -e operator
 Reviewed-by: sla, sspitsyn

 ! test/sun/management/jdp/JdpTest.sh

 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

Re: FAILS: test/sun/management/jdp/JdpTest.sh: test: argument expected

[Dmitry Samersoff]

Chris,

I'm not able to reproduce it locally. Do you have a link to jprt job you
see this failure?

-Dmitry

On 2013-02-12 21:38, Chris Hegarty wrote:
 Dmitry,
 
 This test is now failing on several platforms, on jdk8 and 7u-dev
 
 ---
 
 result: Passed. Compilation successful
 
 #section:shell
 --messages:(3/154)--
 command: shell JdpTest.sh [--jtreg, --no-compile]
 reason: User specified action: run shell JdpTest.sh --jtreg --no-compile
 elapsed time (seconds): 0.045
 --System.out:(0/0)--
 --System.err:(1/110)--
 /export2/Users/chris/repos/jdk8/tl/master_top/jdk/test/sun/management/jdp/JdpTest.sh:
 test: argument expected
 result: Failed. Execution failed: exit code 1
 
 
 test result: Failed. Execution failed: exit code 1
 
 -Chris.
 
 
 On 02/12/2013 12:04 PM, dmitry.samers...@oracle.com wrote:
 Changeset: f7fb173ac833
 Author:dsamersoff
 Date:  2013-02-12 16:02 +0400
 URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f7fb173ac833

 8007786: JDK-8002048 testcase doesn't work on Solaris
 Summary: test built in into Solaris shell doesn't have -e operator
 Reviewed-by: sla, sspitsyn

 ! test/sun/management/jdp/JdpTest.sh



-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* Give Rabbit time, and he'll always get the answer

hg: jdk8/tl/jdk: 8007536: Incorrect copyright header in JDP files

[dmitry . samersoff]

Changeset: 1df991184045
Author:dsamersoff
Date:  2013-02-11 18:44 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/1df991184045

8007536: Incorrect copyright header in JDP files
Summary: Copyright header in JDP files missed the classpath exception rule.
Reviewed-by: mikael

! src/share/classes/sun/management/jdp/JdpBroadcaster.java
! src/share/classes/sun/management/jdp/JdpController.java
! src/share/classes/sun/management/jdp/JdpException.java
! src/share/classes/sun/management/jdp/JdpGenericPacket.java
! src/share/classes/sun/management/jdp/JdpJmxPacket.java
! src/share/classes/sun/management/jdp/JdpPacket.java
! src/share/classes/sun/management/jdp/JdpPacketReader.java
! src/share/classes/sun/management/jdp/JdpPacketWriter.java
! src/share/classes/sun/management/jdp/package-info.java

hg: jdk8/tl/jdk: 8007277: JDK-8002048 testcase fails to compile

[dmitry . samersoff]

Changeset: 0e7d5dd84fdf
Author:dsamersoff
Date:  2013-02-06 16:53 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0e7d5dd84fdf

8007277: JDK-8002048 testcase fails to compile
Summary: sun.* classes is not included to ct.sym file and symbol file have to 
be ignored
Reviewed-by: alanb

! test/sun/management/jdp/JdpTest.sh

hg: jdk8/tl/jdk: 8002048: Protocol to discovery of manageable Java processes on a network

[dmitry . samersoff]

Changeset: 962d6612cace
Author:dsamersoff
Date:  2013-02-03 21:39 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/962d6612cace

8002048: Protocol to discovery of manageable Java processes on a network
Summary: Introduce a protocol to discover manageble Java instances across a 
network subnet, JDP
Reviewed-by: sla, dfuchs

! src/share/classes/sun/management/Agent.java
+ src/share/classes/sun/management/jdp/JdpBroadcaster.java
+ src/share/classes/sun/management/jdp/JdpController.java
+ src/share/classes/sun/management/jdp/JdpException.java
+ src/share/classes/sun/management/jdp/JdpGenericPacket.java
+ src/share/classes/sun/management/jdp/JdpJmxPacket.java
+ src/share/classes/sun/management/jdp/JdpPacket.java
+ src/share/classes/sun/management/jdp/JdpPacketReader.java
+ src/share/classes/sun/management/jdp/JdpPacketWriter.java
+ src/share/classes/sun/management/jdp/package-info.java
+ test/sun/management/jdp/JdpClient.java
+ test/sun/management/jdp/JdpDoSomething.java
+ test/sun/management/jdp/JdpTest.sh
+ test/sun/management/jdp/JdpUnitTest.java

hg: jdk8/tl/jdk: 6783290: MBeanInfo/MBeanFeatureInfo has inconsistent readObject/writeObject

[dmitry . samersoff]

Changeset: b600d490dc57
Author:dsamersoff
Date:  2012-12-20 16:02 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b600d490dc57

6783290: MBeanInfo/MBeanFeatureInfo has inconsistent readObject/writeObject
Summary: call readObject in all cases
Reviewed-by: emcmanus
Contributed-by: jaroslav.bacho...@oracle.com

! src/share/classes/javax/management/MBeanFeatureInfo.java
! src/share/classes/javax/management/MBeanInfo.java

hg: jdk8/tl/jdk: 6937053: RMI unmarshalling errors in ClientNotifForwarder cause silent failure

[dmitry . samersoff]

Changeset: e43f90d5af11
Author:dsamersoff
Date:  2012-12-20 16:56 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e43f90d5af11

6937053: RMI unmarshalling errors in ClientNotifForwarder cause silent failure
Summary: the catch block in the fetchNotifs() method is extended to expect 
UnmarshalException
Reviewed-by: emcmanus
Contributed-by: jaroslav.bacho...@oracle.com

! src/share/classes/com/sun/jmx/remote/internal/ClientNotifForwarder.java

hg: jdk8/tl/jdk: 7009998: JMX synchronization during connection restart is faulty

[dmitry . samersoff]

Changeset: 3f014bc09297
Author:dsamersoff
Date:  2012-12-20 17:24 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3f014bc09297

7009998: JMX synchronization during connection restart is faulty
Summary: add a return statement after the re-connecting has finished and the 
state is CONNECTED
Reviewed-by: sjiang
Contributed-by: jaroslav.bacho...@oracle.com

! make/netbeans/jmx/build.properties
! src/share/classes/com/sun/jmx/remote/internal/ClientCommunicatorAdmin.java

hg: jdk8/tl/jdk: 7186723: TEST_BUG: Race condition in sun/management/jmxremote/startstop/JMXStartStopTest.sh

[dmitry . samersoff]

Changeset: 0c1c4b185451
Author:dsamersoff
Date:  2012-09-29 15:44 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0c1c4b185451

7186723: TEST_BUG: Race condition in 
sun/management/jmxremote/startstop/JMXStartStopTest.sh
Summary: Make test self-terminating and independent to cygwin/mks kill behaviour
Reviewed-by: sspitsyn, alanb

! test/ProblemList.txt
! test/sun/management/jmxremote/startstop/JMXStartStopDoSomething.java
! test/sun/management/jmxremote/startstop/JMXStartStopTest.java
! test/sun/management/jmxremote/startstop/JMXStartStopTest.sh
! test/sun/management/jmxremote/startstop/REMOTE_TESTING.txt

hg: jdk8/tl/jdk: 7194597: Typeo in com.sun.management.VMOption.toString()

[dmitry . samersoff]

Changeset: 2598dad9
Author:dsamersoff
Date:  2012-09-11 19:58 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2598dad9

7194597: Typeo in com.sun.management.VMOption.toString()
Summary: VMOption.toString() returns ...(read-only) if writable 
...(read-write) otherwise.
Reviewed-by: alanb, mchung
Contributed-by: dmytro_she...@hotmail.com

! src/share/classes/com/sun/management/VMOption.java

Re: Not able to complie fresh tl

[Dmitry Samersoff]

Alan,

Sorry for not being clean enough.

tl/jdk/make/java/rmi

repository produce deprecation warning while compiling and these warning
threaten as an error. Therefor build are stopped.

I use 1.8.0-ea-b37 for compilation.

I workarounded the problem with JAVAC_LINT_OPTIONS but would like to
know is it a known issue.

-Dmitry



On 2012-09-03 22:05, Alan Bateman wrote:
 On 03/09/2012 18:42, Dmitry Samersoff wrote:
 Hi Everybody

 Q. Is it (below) known problem?

 Q. Does it make sense to turn deprecation warning off by default or at
 least add an env variable to control it?


 ../../../src/share/classes/sun/rmi/server/ActivatableRef.java:311:
 warning: [deprecation] newCall(RemoteObject,Operation[],int,long) in
 RemoteRef has been deprecated

  public synchronized RemoteCall newCall(RemoteObject obj,


 etc.

 make[2]: Leaving directory `/home/dms/ESC/JSDP/tl/jdk/make/java/rmi'

 jdk8/tl is built and tested on all platforms, including boot cycle
 builds, every night so it mostly be in a healthy state. That said, there
 is currently an issue with OPENJDK=true builds in the corba repository.
 It's not an issue when the closed repos are present. Sean Coffey is
 currently looking into it. From the fragment in your mail then it
 doesn't look like you are running into it. From your mail I can't tell
 if you are just observing deprecation warnings or the warnings are fatal
 (as part of the ongoing effort to reduce javac warnings then they are
 fatal in areas that should be warning free). In any case I would suggest
 bringing your issue to core-libs-dev@openjdk.
 
 -Alan.
 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader

[Dmitry Samersoff]

Mandy,

1. You replace null with getClassLoader() calls in couple of places.
   getClassLoader requires special permissions -
   RuntimePermission(getClassLoader)

   so probably you need to use doPrivileget() there.

   Did you test your changes with SecurityManager/No permissions
   for the test ?


2. Did you consider moving

sm.checkPermission(SecurityConstants.GET_CLASSLOADER_PERMISSION);

inside ClassLoader.needsClassLoaderPermissionCheck(ccl, cl); ?


3. ManagementFactory.java

Could you explain the reason of changes (except 588) ?

-Dmitry


On 2012-08-23 23:33, Mandy Chung wrote:
 On 8/23/2012 11:58 AM, Alan Bateman wrote:
 On 23/08/2012 18:43, Mandy Chung wrote:
 This change is to bring the jdk modularization changes from jigsaw
 repo [1]
 to jdk8.  This allows the jdk modularization changes to be exposed for
 testing as early as possible and minimize the amount of changes carried
 in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.

 Webrev at:
   http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/

 This looks good to me and it's good to have these changes in jdk8.

 One suggestion for ReflectUtil is to add a private static boolean
 isAncestor method as I think that would make the checks in
 needsPackageAccessCheck easier to read. Also in ClassLoader you could
 use just use needsClassLoaderPermissionCheck(from,to).

 
 Done.  This is a good cleanup I considered to do too but missed in the
 previous webrev.
 
 http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/
 
 Thanks for the review.
 Mandy


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...