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: 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
>>  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: 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: 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: 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: 8072379: Implement jdk.Version and jdk.OracleVersion

[Dmitry Samersoff]

Iris,

Did you consider to split version string into array of groups first
(using String.split()), then validate each group separately?

It may make the code better readable and more resilient to possible
future changes.

-Dmitry


On 2015-11-25 04:54, Iris Clark wrote:
> Hi.
> 
> Please review the new classes jdk.Version and jdk.OracleVersion.  These are
> simple Java APIs to parse, validate, and compare version numbers.
> 
>   Bug
> 
> 8072379: Implement jdk.Version and jdk.OracleVersion
> https://bugs.openjdk.java.net/browse/JDK-8072379
> 
>   Webrev
> 
> http://cr.openjdk.java.net/~iris/verona/8072379/webrev.1/
> 
>   JavaDoc
> 
> http://cr.openjdk.java.net/~iris/verona/8072379/doc.1/jdk/Version.html
> 
> http://cr.openjdk.java.net/~iris/verona/8072379/doc.1/jdk/OracleVersion.html
> 
> The .java files are predominantly javadoc. The code is relatively
> straight-forward.
> 
> jdk.Version is the representation of the JDK version string as described in
> JEP 223 ([0], 8061493).  The javadoc is largely taken from the description
> section in the JEP.  The API is described in the "API" section.
> 
> jdk.OracleVersion extends jdk.Version and is the representation of the Oracle
> JDK version string.  Its only purpose is to interpret the fourth element of
> the version number as a patch release.
> 
> There are some minor discrepancies between the implementation and the JEP.
> The JEP will need to be updated.  The most notable is the name of the package
> ("jdk" vs. the original "jdk.util").  The rename was recommended by Mark.
> 
> Thanks,
> iris
>  
> [0] http://openjdk.java.net/jeps/223
> 


-- 
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(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
> ; Java Core Libs
>  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,

We probably can do:

if (fdTable[rootArrayIndex] != NULL) {
   entryTable = fdTable[rootArrayIndex];
}
else { // existing code
  pthread_mutex_lock(&fdTableLock);
  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
> 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  <mailto:thomas.stu...@gmail.com>>; Java Core Libs
> 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/w

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
> 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
> > mailto:thomas.stu...@gmail.com>>; Java
> Core Libs
> >  <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 description.
> >>
> >> The proposed solution

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  <mailto:hbo...@google.com>> wrote:
> 
> 
> On Wed, Mar 2, 2016 at 12:09 AM, Thomas Stüfe 
> 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(&ptr, , memory_order_release);
> 
> and read it with
> 
> atomic_load_explicit(&ptr, 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: (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 
> 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: (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  <mailto:thomas.stu...@gmail.com>> wrote:
> 
> Thank you Roger!
> 
> On Fri, Mar 11, 2016 at 4:26 PM, Roger Riggs  <mailto:roger.ri...@oracle.com>> wrote:
> 
> Hi Thomas,
> 
> When returning the address of the fdentry, the style using
> &fdTable[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
>  <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.
> 
> Memory footprint is kept low: for small values
>

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 {
> +Enumeration 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,

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 {
> +Enumeration 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 {
>>> +Enumeration 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]

Please, file a separate issue.

--Dmitry



-Original Message-
From: Yasumasa Suenaga 
To: Peter Allwin 
Cc: Dmitry Samersoff , 
"core-libs-dev@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  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.
&

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.

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.

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

[Dmitry Samersoff]

(PING) 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)

Use intptr_t everywhere but it costs explicit #ifdef __WINDOWS__ in
splashscreen_stubs.c

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.

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.

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  
> 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.

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]

David,

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?

It's a recommended (from warning-safe point of view) way to deal with
internally defined types because it allows to typedef jobject to
non-pointer type if necessary.

Yes, a bit of paranoia in this case.

>> 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) ?

I tend to agree with you, but it's out of scope of warning cleanup.

It's important for warning cleanup to don't alter code behavior and with
this fix code behaves exactly the same way as it is today - if read
fails, unpack aborts with "CRC error, invalid compressed data" message.

-Dmitry


> 
> 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 
> #include 
> #include 
> #include 
> #include 
> #include 
> 
> #include 
> #include 
> 
> 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]

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: [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(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

y initialized
>> // (in particular the 'next' pointer)
>> U.storeFence();
>> unfinalized = this;
>> }
>> }
>>
>>
>> By doing these modifications, I think you can remove
>> synchronized(lock) {} from printFinalizationQueue().
>>
>>>
>>> I see you are traversing the ‘unfinalized’ list in Finalizer, whereas
>>> the old SA code for ‘-finalizerinfo' traverses the ‘queue’ list. I am
>>> not sure of the difference, perhaps someone from the GC team can help
>>> out?
>>
>> The 'queue' is a ReferenceQueue of Finalizer (FinalReference)
>> instances pending processing by finalizer thread because their
>> referents are eligible for finalization (they are not reachable any
>> more). The 'unfinalized' is a doubly-linked list of all Finalizer
>> instances for which their referents have not been finalized yet
>> (including those 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
>>>>  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: RFR(M,v3): JDK-8059036 : Implement Diagnostic Commands for heap and finalizerinfo

[Dmitry Samersoff]

   head = r;
> 
> Both stores are volatile.
> 
>>
>>> 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(TreeMap) 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
>&g

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):
>>&

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 l

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

[Dmitry Samersoff]

f .next field ( == this or
>>> != this)...
>>>
>>> But I should inspect the GC code too to build a better understanding
>>> of that part of the story...
>>>
>>> Anyway. It turns out that there is already enough state in Reference
>>> to destinguish between it being enqueued as last in list and already
>>> dequeued (inactive) - the additional state is Reference.queue that
>>> transitions from ReferenceQueue.ENQUEUED -> ReferenceQueue.NULL in
>>> ReferenceQueue.reallyPoll. We need to just make sure the two fields
>>> (r.next and r.queue) are assigned and read in correct order. This can
>>> be achieved either by making Reference.next a volatile field or by a
>>> couple of explicit fences:
>>>
>>>
>>> http://cr.openjdk.java.net/~plevart/misc/Finalizer.printFinalizationQueue/webrev.02/
>>>
>>> The assignment of r.queue to ReferenceQueue.ENQUEUED already happens
>>> before linking the reference into the queue's head in
>>> ReferenceQueue.enqueue():
>>>
>>> r.queue = ENQUEUED;
>>> r.next = (head == null) ? r : head;
>>> head = r;
>>>
>>> Both stores are volatile.
>>
>> This sounds a bit better. I don't have a good handle on the pros and
>> cons of a volatile field over explicit fences via Unsafe in this case.
>> Kim might jump in soon.
>>
>> I'd like to suggest an entirely less-clever solution. Since the
>> problem is that forEach() 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
&

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 reallyPoll() {   /* Must hold lock */
> Reference 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
>>>>&g

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.Entry
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
>> 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.Entry
>> 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> 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
>>>> 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,

> 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.Entry[] is already more
> complicated to iterate and navigate in native code. Map
> 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.Entry
>>>> 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> 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
>>>>>> 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  
>> 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 reallyPoll() {   /* Must hold lock */
>>>Reference 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

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
>> 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.Entry
>> 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,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.Entry 
>> On May 19, 2015, at 11:51 PM, Dmitry Samersoff
>> 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.Entry
>> 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, 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 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, 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  
>> 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 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]

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
>>>>  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 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, 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() {
> Map 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  
>>>> 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
>>>>> fi

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() {
>> Map 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  
>>>>> 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

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

done.

> 
> The forEach method - eliminate the use of raw type and
> move @SuppressWarning to the field variable in line 182:
> 
>   @SuppressWarnings("unchecked")
>   Reference 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, 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  
>> 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
>>
>> done.
>>
>>>
>>> The forEach method - eliminate the use of raw type and
>>> move @SuppressWarning to the field variable in line 182:
>>>
>>>  @SuppressWarnings("unchecked")
>>>  Reference 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]

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  wrote:
>>
>>
>>> On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff  
>>> 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 
>>>>>  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.
>>>>
&

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  
>> 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  wrote:
>>>>
>>>>
>>>>> On Jun 4, 2015, at 8:49 AM, Dmitry Samersoff 
>>>>>  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 
>>>>>>>  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.
>>

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 ; 
> core-libs-dev@openjdk.java.net
> Cc: Simonis, Volker 
> 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: [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  wrote:
>>
>> Hi,
>>
>> Thanks for reviewing.
>>
>>> On 30 Oct 2017, at 11:05, Dmitry Samersoff  
>>> 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: [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,

Thank you!

-Dmitry


On 31.10.2017 20:32, Paul Sandoz wrote:
> 
>> On 31 Oct 2017, at 05:58, Dmitry Samersoff  
>> 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  wrote:
>>>>
>>>> Hi,
>>>>
>>>> Thanks for reviewing.
>>>>
>>>>> On 30 Oct 2017, at 11:05, Dmitry Samersoff  
>>>>> 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).
>&g

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: 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: 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 
> 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: 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: 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 ; Mandy Chung
>> ; Chris Bensen 
>> 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(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 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, ®Map);
> -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 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(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(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 ; 'Dmitry Samersoff'
>> ; Java Core Libs > d...@openjdk.java.net>; serviceability-dev (serviceability-
>> d...@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,

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,

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 ; Java Core Libs
>> ; serviceability-dev (serviceability-
>> d...@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 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(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(&attr, &guard_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(&attr,
>>>> 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: JDK 8 RFR 8010371: getaddrinfo can fail with EAI_SYSTEM/EAGAIN, causes UnknownHostException to be thrown

[Dmitry Samersoff]

Brian,

You have to check error != 0 before call to WSAGetLastError() at
ll. 134 Inet6AddressImpl.c

Besides that - the fix looks good for me.

-Dmitry



On 2013-10-14 23:43, Brian Burkhalter wrote:
> 
> On Oct 14, 2013, at 1:58 AM, Alan Bateman wrote:
> 
>>> 2) In Inet4AddressImpl.c and Inet6AddressImpl.c replace
>>> NET_ThrowUnknownHostExceptionWithGaiError with
>>> NET_ThrowByNameWithLastError (see net_md_util.c).
>>>
>>> […]
>>>
>>> If the "con" of option 2 is acceptable then I think that would be the
>>> best way to go, otherwise option 1.
>>>
>> Option #2 seems reasonable, the exception messages for similar network
>> conditions are rarely the same on Windows and Unix anyway.
> 
> Here's the patch updated for this option:
> 
> http://cr.openjdk.java.net/~bpb/8010371/webrev.4/
> 
>> However I think it's important to have verified it with one or two
>> errors to be confident that the errors translate as expected.
> 
> I can do this if we are actually going with this change for JDK 8.
> 
>> One other thing to add is that winsock_errors dates from early
>> versions of Windows whether there wasn't a means to translate Windows
>> Sockets errors. We should look at eliminating it (not for JDK 8 of
>> course, it's too late) so that all errors are handle translated
>> consistently.
> 
> See https://bugs.openjdk.java.net/browse/JDK-4842142.
> 
> 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 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.

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

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

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  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  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.

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 st

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]

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(M): 7147848: [macosx] com.sun.management.UnixOperatingSystem uses hardcoded dummy values

[Dmitry Samersoff]

Staffan,

MacosxOperatingSystem.c

Probably something went wrong with webrev:

 48 if (kr != KERN_SUCCESS) {
 49 return 0;
 50 }


 133 if (gettimeofday(&now, NULL)) {
 134return -1;
 135 }

-Dmitry


On 2012-04-11 11:52, Staffan Larsen wrote:

Thank you all for your comments!

New webrev here: http://cr.openjdk.java.net/~sla/7147848/webrev.01/

Changes:
- Fixed Dmitry's comments
- Updated TestTotalSwap.sh for Darwin (thanks Mandy)

I have verified the values manually by running a Java program (see
attachment) and comparing to top and other system utilities. I've also
compared output with a Linux system to compare magnitude of numbers. I
have run the various regression tests.

Thanks,
/Staffan




On 10 apr 2012, at 21:57, Mandy Chung wrote:


Staffan, Roger,

There isn't any undocumented semantics other than what the
specification for com.sun.management.OperatingSystemMXBean specifies
(that is indicated by its method name):
http://docs.oracle.com/javase/7/docs/jre/api/management/extension/index.html

As Roger suggests, you can do some sanity tests and compare the result
with native commands (top or other CLI). There are a few regression
tests in jdk/test/com/sun/management. In particular, you might want to
update test/com/sun/management/TestTotalSwap.sh to check the swap
space with a suitable macosx command, if there is one.

FYI. I'm not familiar with Mac OS X API and didn't review the code.

Mandy

On 4/10/2012 10:51 AM, rhoover wrote:

Scott, Steffan

(I am he one who put in the original lines of: return -1.0; // not
available being replaced by this patch)

I was reluctant to implement these functions because the linux code
was quite involved and it appeared to me that there might be some
additional semantics to what was implemented than what was indicated
by the function names alone.

I have not compared the code with the 'top' source, but it looks
plausible. As a sanity test, the function values being returned could
be printed by a java program and visually compared with the output of
'top' as a system is loaded up. It might also be wise to run the same
java program on other platforms to make sure that the magnitude of
the numbers is in the same ballpark.

On Apr 10, 2012, at 10:21 AM, Scott Kovatch wrote:


Regarding Apple, Roger Hoover would be a good person to look at
this, as he's spent more time in the Darwin levels of the VM. I
think he's still partially attached to the OpenJDK work.

-- Scott

On Apr 10, 2012, at 8:58 AM, Daniel D. Daugherty wrote:


Staffan,

I reviewed it and I think it looks OK. I tried looking at the code
in MacosxOperatingSystem.c relative to the Linux version, but I think
it is easily possible to miss something subtle here.

You might try a direct ping to Mandy Chung since M&M was her area.
You might also try a direct ping to Mike Swingler to get an Apple
reviewer.

Dan



On 4/10/12 3:30 AM, Staffan Larsen wrote:

Any takers for this review? (added core-libs-dev as well)

Thanks,
/Staffan

On 3 apr 2012, at 15:39, Staffan Larsen wrote:


Please review the following fix:

webrev:
http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>
<http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>
bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7147848

This fix implements the missing functionality in
UnixOperatingSystem for Mac OS X. Any feedback on the
implementation is welcome as I am not very familiar with the APIs
in Mac OS X.

I have verified that the changes build on all platforms through
JPRT. The correctness has been verified manually by looking in
JConsole and running the tests in
test/java/lang/management/OperatingSystemMXBean
test/com/sun/management/OperatingSystemMXBean.

Thanks,
/Staffan





--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: RFR(M): 7147848: [macosx] com.sun.management.UnixOperatingSystem uses hardcoded dummy values

[Dmitry Samersoff]

Looks good for me.

-Dmitry

On 2012-04-11 20:24, Staffan Larsen wrote:

Apparently something went wrong with the webrev. Here is new one:

http://cr.openjdk.java.net/~sla/7147848/webrev.02/

Thanks Dmitry,
/Staffan

On 11 apr 2012, at 12:33, Dmitry Samersoff wrote:


Staffan,

MacosxOperatingSystem.c

Probably something went wrong with webrev:

48 if (kr != KERN_SUCCESS) {
49 return 0;
50 }


133 if (gettimeofday(&now, NULL)) {
134 return -1;
135 }

-Dmitry


On 2012-04-11 11:52, Staffan Larsen wrote:

Thank you all for your comments!

New webrev here: http://cr.openjdk.java.net/~sla/7147848/webrev.01/

Changes:
- Fixed Dmitry's comments
- Updated TestTotalSwap.sh for Darwin (thanks Mandy)

I have verified the values manually by running a Java program (see
attachment) and comparing to top and other system utilities. I've also
compared output with a Linux system to compare magnitude of numbers. I
have run the various regression tests.

Thanks,
/Staffan




On 10 apr 2012, at 21:57, Mandy Chung wrote:


Staffan, Roger,

There isn't any undocumented semantics other than what the
specification for com.sun.management.OperatingSystemMXBean specifies
(that is indicated by its method name):
http://docs.oracle.com/javase/7/docs/jre/api/management/extension/index.html

As Roger suggests, you can do some sanity tests and compare the result
with native commands (top or other CLI). There are a few regression
tests in jdk/test/com/sun/management. In particular, you might want to
update test/com/sun/management/TestTotalSwap.sh to check the swap
space with a suitable macosx command, if there is one.

FYI. I'm not familiar with Mac OS X API and didn't review the code.

Mandy

On 4/10/2012 10:51 AM, rhoover wrote:

Scott, Steffan

(I am he one who put in the original lines of: return -1.0; // not
available being replaced by this patch)

I was reluctant to implement these functions because the linux code
was quite involved and it appeared to me that there might be some
additional semantics to what was implemented than what was indicated
by the function names alone.

I have not compared the code with the 'top' source, but it looks
plausible. As a sanity test, the function values being returned could
be printed by a java program and visually compared with the output of
'top' as a system is loaded up. It might also be wise to run the same
java program on other platforms to make sure that the magnitude of
the numbers is in the same ballpark.

On Apr 10, 2012, at 10:21 AM, Scott Kovatch wrote:


Regarding Apple, Roger Hoover would be a good person to look at
this, as he's spent more time in the Darwin levels of the VM. I
think he's still partially attached to the OpenJDK work.

-- Scott

On Apr 10, 2012, at 8:58 AM, Daniel D. Daugherty wrote:


Staffan,

I reviewed it and I think it looks OK. I tried looking at the code
in MacosxOperatingSystem.c relative to the Linux version, but I think
it is easily possible to miss something subtle here.

You might try a direct ping to Mandy Chung since M&M was her area.
You might also try a direct ping to Mike Swingler to get an Apple
reviewer.

Dan



On 4/10/12 3:30 AM, Staffan Larsen wrote:

Any takers for this review? (added core-libs-dev as well)

Thanks,
/Staffan

On 3 apr 2012, at 15:39, Staffan Larsen wrote:


Please review the following fix:

webrev:
http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>
<http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>
<http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>
<http://cr.openjdk.java.net/~sla/7147848/webrev.00/<http://cr.openjdk.java.net/%7Esla/7147848/webrev.00/>>>
bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7147848

This fix implements the missing functionality in
UnixOperatingSystem for Mac OS X. Any feedback on the
implementation is welcome as I am not very familiar with the APIs
in Mac OS X.

I have verified that the changes build on all platforms through
JPRT. The correctness has been verified manually by looking in
JConsole and running the tests in
test/java/lang/management/OperatingSystemMXBean
test/com/sun/management/OperatingSystemMXBean.

Thanks,
/Staffan





--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...





--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

Deven,

Sorry for stepping latter.

I'll sponsor this fix, but I need some time to take a close look at 
changes as I don't understand clearly why synchronization should help in 
this case.


-Dmitry


On 2012-04-23 23:36, Mandy Chung wrote:

On 04/23/2012 02:36 PM, David Holmes wrote:
 > Except of course that Properties is a Hashtable and synchronizes on
 > 'this' for all public methods. So locking the properties object in the
 > client code will guarantee exclusive access to it.

David, thanks for looking at this closer. I missed that all public
methods of Hashtable are synchronized.

On 4/23/2012 12:52 AM, David Holmes wrote:


You can count me as a Reviewer, but it still needs sign-off from Mandy
as a serviceability representative.



I am not a serviceability representative (I was a member to that group).
I'll defer this to the serviceability team sign off and sponsor your patch.

There are a couple issues with the test. The test needs to be a jtreg
test [1] (basically you need to add the jtreg tags) and should use the
4-space indentation. This new test case should be cleaned up so that it
will exit with and without the bug fix. Your test currently loops
forever when running with the JDK with your patch. This test should be
able to run in samevm mode (other tests may run in the same VM with this
test) and the threads this test starts should be cleaned up before the
test exits.

Mandy
[1] http://openjdk.java.net/jtreg



--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

Deven,

After close look and off-line discussion with David Holmes,
the changes looks good for me.

I'll take care of the rest.

We have one more place in Agent.java executing exactly the same code
so I'll change both of them on your behalf.

-Dmitry


On 2012-04-23 11:43, Deven You wrote:

Thanks David,

So is it ok for you to contribute this patch?

On 04/23/2012 02:36 PM, David Holmes wrote:

Except of course that Properties is a Hashtable and synchronizes on
'this' for all public methods. So locking the properties object in the
client code will guarantee exclusive access to it.

Sorry about that.

David
-

On 23/04/2012 4:30 PM, David Holmes wrote:

Deven,

On 23/04/2012 3:54 PM, Deven You wrote:

On 04/18/2012 02:20 PM, Deven You wrote:

On 04/18/2012 01:34 PM, Mandy Chung wrote:



I think this could still run into CME. System Properties is not a
synchronized map and the setter methods (System.setProperty or
Properties.put method) doesn't synchronize on the Properties object.


The setter methods I'm referring to are System.setProperty and
System.getProperties().put().



I have gone through the Agent.java, I think other set/put methods
related to properties are protected properly.

public static void agentmain using parseString(args) which return a
properties which is a local var and is not possible to cause
concurrent problem when call config_props.putAll(arg_props).

private static synchronized void startLocalManagementAgent() is
synchronized already.

private static synchronized void startRemoteManagementAgent(String
args) is synchronized also.

Could you point where the CME may ocurr?


Is there any suggestion from the mailing list?


The problem is that System.getProperties() returns a globally accessible
set of properties. So even if you prevent the Agent code from modifying
those properties concurrently with other use in the Agent, you have no
such guard for any other piece of code in the system which might also
modify the properties. So the race condition you were trying to fix
still exists. I don't see any way to fix this. No matter what you do
another thread can modify the system properties while you are iterating
them. Instead you need to anticipate the CME and try to recover from it
(also non-trivial).

Cheers,
David Holmes








--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

Deven,

CR number is  7164191 .

Could you re-send me your original e-mail with problem description and
webrev link.

 I'll put it to CR comment field.


-Dmitry



On 2012-04-24 16:15, Dmitry Samersoff wrote:
> Deven,
> 
> After close look and off-line discussion with David Holmes,
> the changes looks good for me.
> 
> I'll take care of the rest.
> 
> We have one more place in Agent.java executing exactly the same code
> so I'll change both of them on your behalf.
> 
> -Dmitry
> 
> 
> On 2012-04-23 11:43, Deven You wrote:
>> Thanks David,
>>
>> So is it ok for you to contribute this patch?
>>
>> On 04/23/2012 02:36 PM, David Holmes wrote:
>>> Except of course that Properties is a Hashtable and synchronizes on
>>> 'this' for all public methods. So locking the properties object in the
>>> client code will guarantee exclusive access to it.
>>>
>>> Sorry about that.
>>>
>>> David
>>> -
>>>
>>> On 23/04/2012 4:30 PM, David Holmes wrote:
>>>> Deven,
>>>>
>>>> On 23/04/2012 3:54 PM, Deven You wrote:
>>>>> On 04/18/2012 02:20 PM, Deven You wrote:
>>>>>> On 04/18/2012 01:34 PM, Mandy Chung wrote:
>>>>>>>
>>>>>>>
>>>>>>> I think this could still run into CME. System Properties is not a
>>>>>>> synchronized map and the setter methods (System.setProperty or
>>>>>>> Properties.put method) doesn't synchronize on the Properties object.
>>>>>>>
>>>>>>>
>>>>>>> The setter methods I'm referring to are System.setProperty and
>>>>>>> System.getProperties().put().
>>>>>>>
>>>>>>
>>>>>> I have gone through the Agent.java, I think other set/put methods
>>>>>> related to properties are protected properly.
>>>>>>
>>>>>> public static void agentmain using parseString(args) which return a
>>>>>> properties which is a local var and is not possible to cause
>>>>>> concurrent problem when call config_props.putAll(arg_props).
>>>>>>
>>>>>> private static synchronized void startLocalManagementAgent() is
>>>>>> synchronized already.
>>>>>>
>>>>>> private static synchronized void startRemoteManagementAgent(String
>>>>>> args) is synchronized also.
>>>>>>
>>>>>> Could you point where the CME may ocurr?
>>>>>
>>>>> Is there any suggestion from the mailing list?
>>>>
>>>> The problem is that System.getProperties() returns a globally
>>>> accessible
>>>> set of properties. So even if you prevent the Agent code from modifying
>>>> those properties concurrently with other use in the Agent, you have no
>>>> such guard for any other piece of code in the system which might also
>>>> modify the properties. So the race condition you were trying to fix
>>>> still exists. I don't see any way to fix this. No matter what you do
>>>> another thread can modify the system properties while you are iterating
>>>> them. Instead you need to anticipate the CME and try to recover from it
>>>> (also non-trivial).
>>>>
>>>> Cheers,
>>>> David Holmes
>>>
>>
>>
> 
> 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

Deven,

Thank you!


On 2012-04-26 05:21, Deven You wrote:
> Hi Dmitry,
> 
> Thanks for your help. I have created a CR with internal id 2236492 which
> hasn't be published yet. So please set this internal CR id as duplicate
> to 716419 as well.
> 
> This is the original mail for this problem:
> 
> 
> 
> ---
> 
> Hi core-libs-devs,
> 
> I am not sure if sun.management.Agent belongs to jmx-dev mailing list,
> if so please anyone tell me.
> 
> This issue is that the sun.management.Agent.loadManagementProperties()
> will invoke properties.putAll which will throw
> ConcurrentModifcationException if there are other threads which modify
> the properties concurrently.
> 
> I have made a patch[1] which synchronize the sysProps so that putAll can
> work on multi-thread scenario. The test case is also available in [1].
> 
> Thanks a lot!
> 
> [1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00
> <http://cr.openjdk.java.net/%7Elittlee/OJDK-256/webrev.00>
> 
> ------
> 
> 
> Thanks a lot!
> 
> 
> 
> On 04/25/2012 08:32 PM, Dmitry Samersoff wrote:
>> Deven,
>>
>> CR number is  7164191 .
>>
>> Could you re-send me your original e-mail with problem description and
>> webrev link.
>>
>>   I'll put it to CR comment field.
>>
>>
>> -Dmitry
>>
>>
>>
>> On 2012-04-24 16:15, Dmitry Samersoff wrote:
>>> Deven,
>>>
>>> After close look and off-line discussion with David Holmes,
>>> the changes looks good for me.
>>>
>>> I'll take care of the rest.
>>>
>>> We have one more place in Agent.java executing exactly the same code
>>> so I'll change both of them on your behalf.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2012-04-23 11:43, Deven You wrote:
>>>> Thanks David,
>>>>
>>>> So is it ok for you to contribute this patch?
>>>>
>>>> On 04/23/2012 02:36 PM, David Holmes wrote:
>>>>> Except of course that Properties is a Hashtable and synchronizes on
>>>>> 'this' for all public methods. So locking the properties object in the
>>>>> client code will guarantee exclusive access to it.
>>>>>
>>>>> Sorry about that.
>>>>>
>>>>> David
>>>>> -
>>>>>
>>>>> On 23/04/2012 4:30 PM, David Holmes wrote:
>>>>>> Deven,
>>>>>>
>>>>>> On 23/04/2012 3:54 PM, Deven You wrote:
>>>>>>> On 04/18/2012 02:20 PM, Deven You wrote:
>>>>>>>> On 04/18/2012 01:34 PM, Mandy Chung wrote:
>>>>>>>>>
>>>>>>>>> I think this could still run into CME. System Properties is not a
>>>>>>>>> synchronized map and the setter methods (System.setProperty or
>>>>>>>>> Properties.put method) doesn't synchronize on the Properties
>>>>>>>>> object.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The setter methods I'm referring to are System.setProperty and
>>>>>>>>> System.getProperties().put().
>>>>>>>>>
>>>>>>>> I have gone through the Agent.java, I think other set/put methods
>>>>>>>> related to properties are protected properly.
>>>>>>>>
>>>>>>>> public static void agentmain using parseString(args) which return a
>>>>>>>> properties which is a local var and is not possible to cause
>>>>>>>> concurrent problem when call config_props.putAll(arg_props).
>>>>>>>>
>>>>>>>> private static synchronized void startLocalManagementAgent() is
>>>>>>>> synchronized already.
>>>>>>>>
>>>>>>>> private static synchronized void startRemoteManagementAgent(String
>>>>>>>> args) is synchronized also.
>>>>>>>>
>>>>>>>> Could you point where the CME may ocurr?
>>>>>>> Is there any suggestion from the mailing list?
>>>>>> The problem is that System.getProperties() returns a globally
>>>>>> accessible
>>>>>> set of properties. So even if you prevent the Agent code from
>>>>>> modifying
>>>>>> those properties concurrently with other use in the Agent, you
>>>>>> have no
>>>>>> such guard for any other piece of code in the system which might also
>>>>>> modify the properties. So the race condition you were trying to fix
>>>>>> still exists. I don't see any way to fix this. No matter what you do
>>>>>> another thread can modify the system properties while you are
>>>>>> iterating
>>>>>> them. Instead you need to anticipate the CME and try to recover
>>>>>> from it
>>>>>> (also non-trivial).
>>>>>>
>>>>>> Cheers,
>>>>>> David Holmes
>>>>
>>>
>>
> 
> 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

Re: sun.management.Agent: the properties.putAll API may fail with ConcurrentModifcationException on multi-thread scenario

[Dmitry Samersoff]

Deven,

The fix is submitted to openjdk  tl workspace

  http://hg.openjdk.java.net/jdk8/tl/jdk/rev/df33f5f750ec

-Dmitry


On 2012-04-26 05:21, Deven You wrote:
> Hi Dmitry,
> 
> Thanks for your help. I have created a CR with internal id 2236492 which
> hasn't be published yet. So please set this internal CR id as duplicate
> to 716419 as well.
> 
> This is the original mail for this problem:
> 
> 
> 
> ---
> 
> Hi core-libs-devs,
> 
> I am not sure if sun.management.Agent belongs to jmx-dev mailing list,
> if so please anyone tell me.
> 
> This issue is that the sun.management.Agent.loadManagementProperties()
> will invoke properties.putAll which will throw
> ConcurrentModifcationException if there are other threads which modify
> the properties concurrently.
> 
> I have made a patch[1] which synchronize the sysProps so that putAll can
> work on multi-thread scenario. The test case is also available in [1].
> 
> Thanks a lot!
> 
> [1] http://cr.openjdk.java.net/~littlee/OJDK-256/webrev.00
> <http://cr.openjdk.java.net/%7Elittlee/OJDK-256/webrev.00>
> 
> ------
> 
> 
> Thanks a lot!
> 
> 
> 
> On 04/25/2012 08:32 PM, Dmitry Samersoff wrote:
>> Deven,
>>
>> CR number is  7164191 .
>>
>> Could you re-send me your original e-mail with problem description and
>> webrev link.
>>
>>   I'll put it to CR comment field.
>>
>>
>> -Dmitry
>>
>>
>>
>> On 2012-04-24 16:15, Dmitry Samersoff wrote:
>>> Deven,
>>>
>>> After close look and off-line discussion with David Holmes,
>>> the changes looks good for me.
>>>
>>> I'll take care of the rest.
>>>
>>> We have one more place in Agent.java executing exactly the same code
>>> so I'll change both of them on your behalf.
>>>
>>> -Dmitry
>>>
>>>
>>> On 2012-04-23 11:43, Deven You wrote:
>>>> Thanks David,
>>>>
>>>> So is it ok for you to contribute this patch?
>>>>
>>>> On 04/23/2012 02:36 PM, David Holmes wrote:
>>>>> Except of course that Properties is a Hashtable and synchronizes on
>>>>> 'this' for all public methods. So locking the properties object in the
>>>>> client code will guarantee exclusive access to it.
>>>>>
>>>>> Sorry about that.
>>>>>
>>>>> David
>>>>> -
>>>>>
>>>>> On 23/04/2012 4:30 PM, David Holmes wrote:
>>>>>> Deven,
>>>>>>
>>>>>> On 23/04/2012 3:54 PM, Deven You wrote:
>>>>>>> On 04/18/2012 02:20 PM, Deven You wrote:
>>>>>>>> On 04/18/2012 01:34 PM, Mandy Chung wrote:
>>>>>>>>>
>>>>>>>>> I think this could still run into CME. System Properties is not a
>>>>>>>>> synchronized map and the setter methods (System.setProperty or
>>>>>>>>> Properties.put method) doesn't synchronize on the Properties
>>>>>>>>> object.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The setter methods I'm referring to are System.setProperty and
>>>>>>>>> System.getProperties().put().
>>>>>>>>>
>>>>>>>> I have gone through the Agent.java, I think other set/put methods
>>>>>>>> related to properties are protected properly.
>>>>>>>>
>>>>>>>> public static void agentmain using parseString(args) which return a
>>>>>>>> properties which is a local var and is not possible to cause
>>>>>>>> concurrent problem when call config_props.putAll(arg_props).
>>>>>>>>
>>>>>>>> private static synchronized void startLocalManagementAgent() is
>>>>>>>> synchronized already.
>>>>>>>>
>>>>>>>> private static synchronized void startRemoteManagementAgent(String
>>>>>>>> args) is synchronized also.
>>>>>>>>
>>>>>>>> Could you point where the CME may ocurr?
>>>>>>> Is there any suggestion from the mailing list?
>>>>>> The problem is that System.getProperties() returns a globally
>>>>>> accessible
>>>>>> set of properties. So even if you prevent the Agent code from
>>>>>> modifying
>>>>>> those properties concurrently with other use in the Agent, you
>>>>>> have no
>>>>>> such guard for any other piece of code in the system which might also
>>>>>> modify the properties. So the race condition you were trying to fix
>>>>>> still exists. I don't see any way to fix this. No matter what you do
>>>>>> another thread can modify the system properties while you are
>>>>>> iterating
>>>>>> them. Instead you need to anticipate the CME and try to recover
>>>>>> from it
>>>>>> (also non-trivial).
>>>>>>
>>>>>> Cheers,
>>>>>> David Holmes
>>>>
>>>
>>
> 
> 


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...

hg: jdk8/tl/jdk: 7183753: [TEST] Some colon in the diff for this test

[dmitry . samersoff]

Changeset: bf85c3ab2637
Author:dsamersoff
Date:  2012-08-09 14:52 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bf85c3ab2637

7183753: [TEST] Some colon in the diff for this test
Summary: Reference output file contains extra colon
Reviewed-by: sspitsyn, mgronlun

! test/sun/tools/jcmd/help_help.out

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 ...

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 ...

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

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: 6809322: javax.management.timer.Timer does not fire all notifications

[dmitry . samersoff]

Changeset: b2d8a99a049e
Author:dsamersoff
Date:  2012-10-17 18:34 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b2d8a99a049e

6809322: javax.management.timer.Timer does not fire all notifications
Summary: Some notifications get dropped due to ConcurrentModificationException 
thrown in Timer.notifyAlarmClock() method.
Reviewed-by: dholmes, rbackman
Contributed-by: jaroslav.bacho...@oracle.com

! src/share/classes/javax/management/timer/Timer.java
+ test/javax/management/timer/MissingNotificationTest.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: 8005309: Missed tests for 6783290,6937053,7009998

[dmitry . samersoff]

Changeset: c1a55ee9618e
Author:dsamersoff
Date:  2012-12-20 20:12 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c1a55ee9618e

8005309: Missed tests for 6783290,6937053,7009998
Summary: Missed tests for 6783290,6937053,7009998
Reviewed-by: sjiang, emcmanus
Contributed-by: jaroslav.bacho...@oracle.com

+ test/com/sun/jmx/remote/CCAdminReconnectTest.java
+ test/com/sun/jmx/remote/NotificationMarshalVersions/Client/Client.java
+ test/com/sun/jmx/remote/NotificationMarshalVersions/Client/ConfigKey.java
+ 
test/com/sun/jmx/remote/NotificationMarshalVersions/Client/TestNotification.java
+ test/com/sun/jmx/remote/NotificationMarshalVersions/Server/ConfigKey.java
+ test/com/sun/jmx/remote/NotificationMarshalVersions/Server/Server.java
+ test/com/sun/jmx/remote/NotificationMarshalVersions/Server/Ste.java
+ test/com/sun/jmx/remote/NotificationMarshalVersions/Server/SteMBean.java
+ 
test/com/sun/jmx/remote/NotificationMarshalVersions/Server/TestNotification.java
+ 
test/com/sun/jmx/remote/NotificationMarshalVersions/TestSerializationMismatch.sh
+ test/javax/management/MBeanInfo/SerializationTest1.java

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: 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: 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: 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