Re: RFR: 8075667: (tz) Support tzdata2015b

[Masayoshi Okutsu]

Looks good except that data changes keep requiring additional workaround 
to the run-time code. We do need a fix for JDK-8014468.


Masayoshi

On 3/27/2015 12:47 AM, Aleksej Efimov wrote:

Hi,

Please review the fix for adding the latest tzdata2015b to JDK9.
The JTREG and JPRT tests were executed.
No failures were observed, except one:
test/sun/util/calendar/zi/TestZoneInfo310.java
The failure was caused by the following tzdb rule addition to 
make/data/tzdata/asia:
+Rule Palestine2015max-MarlastFri24:00 
 1:00S
And it was addressed by addition of another piece of workaround code 
to src/java.base/share/classes/sun/util/calendar/ZoneInfoFile.java.

After this changes the test showed the PASS result.

Thank you,
Aleksej

[1] JBS: https://bugs.openjdk.java.net/browse/JDK-8075667
[2] Webrev: http://cr.openjdk.java.net/~aefimov/tzdata/2015b/9/00

Re: Review request for JDK-8051560: JAXP function astro tests conversion

[huizhe wang]

Please also describe the mapping of the original tests and the new ones 
so that we are sure no test cases are lost in the transition.


Thanks,
Joe

On 3/26/2015 6:01 PM, huizhe wang wrote:
I second Lance. The Main of the original astro had Javadocs and 
developer comments. Probably more important is that you've completely 
changed the main classes (TestDriver and Main), which looks good, 
however, the original classes contained a lot of information on what 
each test does and how it works that seem to have all been lost.


The suite's README and build.xml may also contain information that is 
worth keeping. Some of them may be no longer valid, in which case, it 
may be helpful to describe the change. For example, the log files 
described in README were useful for debugging. It would be good to 
explain where to find the debug info in your new design.


While scanning the test, I see that you are creating temporary output 
files under USER_DIR. Is that intended? JAXP processors do not 
necessarily open them with the DELETE_ON_CLOSE option. I thought in 
previous tests, you were creating them in the scratch directory.


Thanks,
Joe

On 3/26/2015 12:08 PM, Lance Andersen wrote:

Hi Frank,

Overall these look fine.  I would suggest adding a simple comment to 
describe the tests that do not have one to give a basic intent of the 
test to make it easier for someone to understand if they are new.


Best
Lance
On Mar 25, 2015, at 5:34 AM, Frank Yuan > wrote:



Hi, Joe and All



We are working on moving internal jaxp functional tests to open jdk 
repo.


This is the astro suite. Would you please review these test? Any 
comment

will be appreciated.



bug: https://bugs.openjdk.java.net/browse/JDK-8051560

webrev: http://cr.openjdk.java.net/~fyuan/8051560/webrev.00/ 





AstroTest is the primary test in this suite, it transforms an xml 
file(which

includes astro data) with several xsl files, sets different filtering
condition by these xsl files and different filtering range, finally 
compares

the result with golden files.

And there are 5 permutations of InputSourceFactory and 
FilterFactory(I uses

template method pattern for the variant FilterFactoryImpls), each
permutation will be applied to above transforming processes.



Thanks,



Frank




 

Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: Review request for JDK-8051560: JAXP function astro tests conversion

[huizhe wang]

I second Lance. The Main of the original astro had Javadocs and 
developer comments. Probably more important is that you've completely 
changed the main classes (TestDriver and Main), which looks good, 
however, the original classes contained a lot of information on what 
each test does and how it works that seem to have all been lost.


The suite's README and build.xml may also contain information that is 
worth keeping. Some of them may be no longer valid, in which case, it 
may be helpful to describe the change. For example, the log files 
described in README were useful for debugging. It would be good to 
explain where to find the debug info in your new design.


While scanning the test, I see that you are creating temporary output 
files under USER_DIR. Is that intended? JAXP processors do not 
necessarily open them with the DELETE_ON_CLOSE option. I thought in 
previous tests, you were creating them in the scratch directory.


Thanks,
Joe

On 3/26/2015 12:08 PM, Lance Andersen wrote:

Hi Frank,

Overall these look fine.  I would suggest adding a simple comment to 
describe the tests that do not have one to give a basic intent of the 
test to make it easier for someone to  understand if they are new.


Best
Lance
On Mar 25, 2015, at 5:34 AM, Frank Yuan > wrote:



Hi, Joe and All



We are working on moving internal jaxp functional tests to open jdk repo.

This is the astro suite. Would you please review these test?  Any comment
will be appreciated.



bug: https://bugs.openjdk.java.net/browse/JDK-8051560

webrev: http://cr.openjdk.java.net/~fyuan/8051560/webrev.00/ 





AstroTest is the primary test in this suite, it transforms an xml 
file(which

includes astro data) with several xsl files, sets different filtering
condition by these xsl files and different filtering range, finally 
compares

the result with golden files.

And there are 5 permutations of InputSourceFactory and 
FilterFactory(I uses

template method pattern for the variant FilterFactoryImpls), each
permutation will be applied to above transforming processes.



Thanks,



Frank





Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: 8074297: substring in XSLT returns wrong character if string contains supplementary chars

[huizhe wang]

Hi Aleksej,

Looks good.

Thanks,
Joe

On 3/26/2015 9:20 AM, Aleksej Efimov wrote:

Hello,
Please review the fix for XSLT 'substring' function that solves the 
problem when string parameter contains supplementary characters.
The 'test/javax/xml/jaxp/transform/8062923/XslSubstringTest.java' test 
was updated to include the test cases for fail behavior. Fixed 
function utilizes the
codePoint functions to calculate String offsets and length, similar to 
'string-length' XSL function.
Testing results: JTREG and JPRT shows no failures with XML related 
tests. Modified test shows no failures.


With Best Regards,
Aleksej

[1] JBS: https://bugs.openjdk.java.net/browse/JDK-8074297
[2] Webrevs:
jaxp: http://cr.openjdk.java.net/~aefimov/8074297/9/00/jaxp/
jdk: http://cr.openjdk.java.net/~aefimov/8074297/9/00/jdk/

Re: [8] RFR of 8066985: Java Webstart downloading packed files can result in Timezone set to UTC

[mikhail cherkasov]

On 3/25/2015 6:19 PM, Kumar Srinivasan wrote:

should we throttle/adapt the test on the number of processors available
using Runtime.availableProcessors() ?

Like this http://cr.openjdk.java.net/~mcherkas/8066985/webrev.07 ?

Re: Review request for JDK-8051560: JAXP function astro tests conversion

[Lance Andersen]

Hi Frank,

Overall these look fine.  I would suggest adding a simple comment to describe 
the tests that do not have one to give a basic intent of the test to make it 
easier for someone to  understand if they are new.

Best
Lance
On Mar 25, 2015, at 5:34 AM, Frank Yuan  wrote:

> Hi, Joe and All
> 
> 
> 
> We are working on moving internal jaxp functional tests to open jdk repo.
> 
> This is the astro suite. Would you please review these test?  Any comment
> will be appreciated.
> 
> 
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8051560
> 
> webrev: http://cr.openjdk.java.net/~fyuan/8051560/webrev.00/
> 
> 
> 
> AstroTest is the primary test in this suite, it transforms an xml file(which
> includes astro data) with several xsl files, sets different filtering
> condition by these xsl files and different filtering range, finally compares
> the result with golden files. 
> 
> And there are 5 permutations of InputSourceFactory and FilterFactory(I uses
> template method pattern for the variant FilterFactoryImpls), each
> permutation will be applied to above transforming processes.
> 
> 
> 
> Thanks,
> 
> 
> 
> Frank
> 



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Daniel Fuchs]

On 26/03/15 15:18, Peter Levart wrote:

On 03/26/2015 02:55 PM, Daniel Fuchs wrote:

On 26/03/15 14:43, Peter Levart wrote:

On 03/26/2015 01:28 PM, David Holmes wrote:

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
  instead of Hashtable


As you say in the bug report, now that the map is always accessed
within synchronized code this serves no purpose. The map not only
doesn't need to be concurrent, it doesn't even need to be thread-safe!
So why not replace with a simple HashMap?

David


Hi Daniel,

...or, if you keep CHM, you might be able to use it for a benefit.

You say that retrieving a logger by name is the most frequent operation.
If you can prove that moving a statement in addLocalLogger method that
publishes the Logger via CHM to the end of synchronized block without
hurting the logic that exists in-between and initializes the Logger
instance, then you can implement the findLogger method in a way that
almost never needs to enter synchronized block:

http://cr.openjdk.java.net/~plevart/jdk9-dev/LogManager.CHM/webrev.01/


Right. I was also thinking that there may be a way to
use computeIfAbsent to rewrite some of the old logic - but I'd
rather do that in a separate change set.
This is partly why I didn't want to go much more beyond
the simple switch here. I'll keep your idea above in mind though.

Synchronization in LogManager is something that has always proved
to require careful thinking ;-)
Is it OK with you if I log a follow up RFE instead?


Yes, by all means.

CHM.computeIfAbsent() does use a lock, but it's a lock on a hash-bin, so
it's tricky if your logic is re-entrant (CHM.computIfAbsent is not
reentrant). Just exposing a get() without synchronization like I
proposed in findLogger() should be pretty safe if you do the publication
of new Logger instance as the last action in addLocalLogger synchronized
block. Those methods that change state in the context are better left
synchronized on the context instance as they must do several mutations
atomically involving various kinds of objects (Nodes, Loggers) that are
interconnected. And you don't loose much by synchronizing as creating
new Loggers is not a frequent operation and you can keep the logic plain
and simple. I think it's only worth optimizing for common case (the
findLogger()).



Hi Peter,

OK - you almost convinced me. I'm experimenting your idea.
There are a few tricky parts - such as moving the publication
of the logger to the last statement in addLocalLogger, but
fortunately some of the existing tests catch that if you
do it wrong ;-)

That said, the 'common' case is - I think - to call
Logger.getLogger() only once when you create the logger.
So in that common case, findLogger finds nothing - but then
addLocalLogger is called and so synchronization still has to
happen.

I'll send an updated webrev if I see that tests on all platform
succeed. We can then decide whether that's worth it :-)

Thanks for the reviews and ideas!

-- daniel




Regards, Peter



best regards

-- daniel


Regards, Peter




https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

Re: RFR 8076039: Remove the unused internal API sun.reflect.misc.FieldUtil.getDeclaredFields

[Mandy Chung]

On 3/25/15 11:55 PM, Srikanth wrote:

Greetings,

Please review this patch to remove the said unused internal API - this 
has been
reviewed by a couple of engineers already and is now submitted now for 
wider
review. I have built JDK on all platforms and ran the tests to make 
sure this is safe.


JBS:https://bugs.openjdk.java.net/browse/JDK-8076039
Webrev: http://cr.openjdk.java.net/~sadayapalam/8076039/webrev/


Looks fine to me.  I can sponsor and push this patch for you.

Mandy

RFR 8076039: Remove the unused internal API sun.reflect.misc.FieldUtil.getDeclaredFields

[Srikanth]

Greetings,

Please review this patch to remove the said unused internal API - this 
has been
reviewed by a couple of engineers already and is now submitted now for 
wider
review. I have built JDK on all platforms and ran the tests to make sure 
this is safe.


JBS:https://bugs.openjdk.java.net/browse/JDK-8076039
Webrev: http://cr.openjdk.java.net/~sadayapalam/8076039/webrev/

Thanks!
Srikanth

Re: RFR: JDK-8073158 zip files with total entry count 0xFFFF need not be ZIP64 files

[Martin Buchholz]

I just tried another clean rebuild and jtreg on jar/zip and launcher tests
with -ignore:run - no problems on Linux x86.

On Thu, Mar 26, 2015 at 7:49 AM, Kumar Srinivasan <
kumar.x.sriniva...@oracle.com> wrote:

> Hi Martin,
>
> In case you have missed it, jdk/test/tools/launcher/BigJar.java
> has a suppressed large file test, you might want to enable it
> if not already done, I think this test originally came from google
> not sure, nevertheless,  a similar one also exists in:
> langtools/test/tools/javac/file/zip/T6836682.java
>
> As for the changes I glosssed over it, yea it is scary, but thanks for
> making
> the changes.
>
>  The Right Thing is to have only one C Zip implementation, shared by
>> launcher, zip_util, and pack200.  And my change is one step towards that,
>> but I'm not tackling the big job right now.
>>
> +1.
>
>
>
> Kumar
>
>
>> On Wed, Mar 25, 2015 at 10:55 AM, Xueming Shen 
>> wrote:
>>
>>  It looks fine...I hope you guys also have some tests over there to bring
>>> in more confidence :-)
>>>
>>> It might be "easier" to simply update the original haveZIP64() with the
>>> code
>>> we have in zip_util.c in which we also try to read the end64 to verify if
>>> we
>>> really have one. Your choice though.
>>>
>>> -Sherman
>>>
>>>
>>> On 03/25/2015 09:55 AM, Martin Buchholz wrote:
>>>
>>> Yeah, this review is kinda scary.  There's a lot of technical debt here,
>>> and this change only addresses some of it.
>>>
>>>   A variant of this code is in use at Google.
>>>
>>> On Mon, Mar 23, 2015 at 1:18 PM, Martin Buchholz 
>>> wrote:
>>>
>>>Hi Xueming and Alan,

   I'd like you to do a code review.

   https://bugs.openjdk.java.net/browse/JDK-8073158

 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/
 0x-entries-zip-file/

   Of course, the really correct thing is to have at most one zip
 implementation per programming language, but I'm not trying to fix that
 here (how many zip implementations does openjdk have?)


>>>
>>>
>

RFR: 8074297: substring in XSLT returns wrong character if string contains supplementary chars

[Aleksej Efimov]

Hello,
Please review the fix for XSLT 'substring' function that solves the 
problem when string parameter contains supplementary characters.
The 'test/javax/xml/jaxp/transform/8062923/XslSubstringTest.java' test 
was updated to include the test cases for fail behavior. Fixed function 
utilizes the
codePoint functions to calculate String offsets and length, similar to 
'string-length' XSL function.
Testing results: JTREG and JPRT shows no failures with XML related 
tests. Modified test shows no failures.


With Best Regards,
Aleksej

[1] JBS: https://bugs.openjdk.java.net/browse/JDK-8074297
[2] Webrevs:
jaxp: http://cr.openjdk.java.net/~aefimov/8074297/9/00/jaxp/
jdk: http://cr.openjdk.java.net/~aefimov/8074297/9/00/jdk/

RFR: 8075667: (tz) Support tzdata2015b

[Aleksej Efimov]

Hi,

Please review the fix for adding the latest tzdata2015b to JDK9.
The JTREG and JPRT tests were executed.
No failures were observed, except one:
test/sun/util/calendar/zi/TestZoneInfo310.java
The failure was caused by the following tzdb rule addition to 
make/data/tzdata/asia:
+Rule Palestine2015max-MarlastFri24:00 
 1:00S
And it was addressed by addition of another piece of workaround code to 
src/java.base/share/classes/sun/util/calendar/ZoneInfoFile.java.

After this changes the test showed the PASS result.

Thank you,
Aleksej

[1] JBS: https://bugs.openjdk.java.net/browse/JDK-8075667
[2] Webrev: http://cr.openjdk.java.net/~aefimov/tzdata/2015b/9/00

RFR: 8073385: Bad error message on parsing illegal character in XML attribute

[Aleksej Efimov]

Hi,
Please review the fix for corrupted error messages in XML parser. When 
the XML parser encounters illegal

character the message with incorrect data is generated:
An invalid XML character (Unicode: 0x{2}) was found in the value of 
attribute "{1}" and element is "0".

But it should be like:
An invalid XML character (Unicode: 0x0) was found in the value of 
attribute "attTest" and element is "topElement".
The fix repairs the message generation and it is similar to the code in 
Apache Xerces project.
Testing: JTREG and JPRT tests (with new one) showed no failures with XML 
related tests.


Thanks,
Aleksej

[1] JBS: https://bugs.openjdk.java.net/browse/JDK-8073385
[2] Webrevs:
jaxp: http://cr.openjdk.java.net/~aefimov/8073385/9/00/jaxp/
jdk: http://cr.openjdk.java.net/~aefimov/8073385/9/00/jdk/

Re: RFR: JDK-8073158 zip files with total entry count 0xFFFF need not be ZIP64 files

[Kumar Srinivasan]

Hi Martin,

In case you have missed it, jdk/test/tools/launcher/BigJar.java
has a suppressed large file test, you might want to enable it
if not already done, I think this test originally came from google
not sure, nevertheless,  a similar one also exists in:
langtools/test/tools/javac/file/zip/T6836682.java

As for the changes I glosssed over it, yea it is scary, but thanks for 
making

the changes.


The Right Thing is to have only one C Zip implementation, shared by
launcher, zip_util, and pack200.  And my change is one step towards that,
but I'm not tackling the big job right now.

+1.



Kumar


On Wed, Mar 25, 2015 at 10:55 AM, Xueming Shen 
wrote:


It looks fine...I hope you guys also have some tests over there to bring
in more confidence :-)

It might be "easier" to simply update the original haveZIP64() with the
code
we have in zip_util.c in which we also try to read the end64 to verify if
we
really have one. Your choice though.

-Sherman


On 03/25/2015 09:55 AM, Martin Buchholz wrote:

Yeah, this review is kinda scary.  There's a lot of technical debt here,
and this change only addresses some of it.

  A variant of this code is in use at Google.

On Mon, Mar 23, 2015 at 1:18 PM, Martin Buchholz 
wrote:


  Hi Xueming and Alan,

  I'd like you to do a code review.

  https://bugs.openjdk.java.net/browse/JDK-8073158

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/0x-entries-zip-file/

  Of course, the really correct thing is to have at most one zip
implementation per programming language, but I'm not trying to fix that
here (how many zip implementations does openjdk have?)

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Daniel Fuchs]

On 26/03/15 14:32, Daniel Fuchs wrote:

The enumeration of keys returned from the map will be iterated
outside of any synchronized block.
ConcurrentHashMap makes this safe.


I'm happy to report that the test in the webrev [1] fails in
ConcurrentModificationException if HashMap is used instead
of ConcurrentHashMap :-)

best regards

-- daniel

[1]
http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Peter Levart]

On 03/26/2015 02:55 PM, Daniel Fuchs wrote:

On 26/03/15 14:43, Peter Levart wrote:

On 03/26/2015 01:28 PM, David Holmes wrote:

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
  instead of Hashtable


As you say in the bug report, now that the map is always accessed
within synchronized code this serves no purpose. The map not only
doesn't need to be concurrent, it doesn't even need to be thread-safe!
So why not replace with a simple HashMap?

David


Hi Daniel,

...or, if you keep CHM, you might be able to use it for a benefit.

You say that retrieving a logger by name is the most frequent operation.
If you can prove that moving a statement in addLocalLogger method that
publishes the Logger via CHM to the end of synchronized block without
hurting the logic that exists in-between and initializes the Logger
instance, then you can implement the findLogger method in a way that
almost never needs to enter synchronized block:

http://cr.openjdk.java.net/~plevart/jdk9-dev/LogManager.CHM/webrev.01/


Right. I was also thinking that there may be a way to
use computeIfAbsent to rewrite some of the old logic - but I'd
rather do that in a separate change set.
This is partly why I didn't want to go much more beyond
the simple switch here. I'll keep your idea above in mind though.

Synchronization in LogManager is something that has always proved
to require careful thinking ;-)
Is it OK with you if I log a follow up RFE instead?


Yes, by all means.

CHM.computeIfAbsent() does use a lock, but it's a lock on a hash-bin, so 
it's tricky if your logic is re-entrant (CHM.computIfAbsent is not 
reentrant). Just exposing a get() without synchronization like I 
proposed in findLogger() should be pretty safe if you do the publication 
of new Logger instance as the last action in addLocalLogger synchronized 
block. Those methods that change state in the context are better left 
synchronized on the context instance as they must do several mutations 
atomically involving various kinds of objects (Nodes, Loggers) that are 
interconnected. And you don't loose much by synchronizing as creating 
new Loggers is not a frequent operation and you can keep the logic plain 
and simple. I think it's only worth optimizing for common case (the 
findLogger()).


Regards, Peter



best regards

-- daniel


Regards, Peter




https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Daniel Fuchs]

On 26/03/15 14:42, Jason Mehrens wrote:

The snapshot enumeration is a welcomed change.  ConcurrentHashMap has legacy 
Hashtable methods so you can save a little bit by calling namedLoggers.keys() 
instead of wrapping the key set.


Yes - but the enumeration returned by ConcurrentHashMap.keys
is in fact an iterator which will let you remove keys.
I explicitly chose to use Collections.enumeration instead
to prevent providing unintended access.


best regards,

-- daniel




Jason



Date: Thu, 26 Mar 2015 14:32:23 +0100
From: daniel.fu...@oracle.com
To: david.hol...@oracle.com; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 7113878: LogManager - namedLoggers should be 
ConcurrentHashMap instead of Hashtable

On 26/03/15 13:28, David Holmes wrote:

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
instead of Hashtable


As you say in the bug report, now that the map is always accessed within
synchronized code this serves no purpose. The map not only doesn't need
to be concurrent, it doesn't even need to be thread-safe! So why not
replace with a simple HashMap?


You found me out ;-) I should have mentioned it.
The enumeration of keys returned from the map will be iterated
outside of any synchronized block.
ConcurrentHashMap makes this safe.

best regards,

-- daniel




David


https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Daniel Fuchs]

On 26/03/15 14:43, Peter Levart wrote:

On 03/26/2015 01:28 PM, David Holmes wrote:

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
  instead of Hashtable


As you say in the bug report, now that the map is always accessed
within synchronized code this serves no purpose. The map not only
doesn't need to be concurrent, it doesn't even need to be thread-safe!
So why not replace with a simple HashMap?

David


Hi Daniel,

...or, if you keep CHM, you might be able to use it for a benefit.

You say that retrieving a logger by name is the most frequent operation.
If you can prove that moving a statement in addLocalLogger method that
publishes the Logger via CHM to the end of synchronized block without
hurting the logic that exists in-between and initializes the Logger
instance, then you can implement the findLogger method in a way that
almost never needs to enter synchronized block:

http://cr.openjdk.java.net/~plevart/jdk9-dev/LogManager.CHM/webrev.01/


Right. I was also thinking that there may be a way to
use computeIfAbsent to rewrite some of the old logic - but I'd
rather do that in a separate change set.
This is partly why I didn't want to go much more beyond
the simple switch here. I'll keep your idea above in mind though.

Synchronization in LogManager is something that has always proved
to require careful thinking ;-)
Is it OK with you if I log a follow up RFE instead?

best regards

-- daniel


Regards, Peter




https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Peter Levart]

On 03/26/2015 01:28 PM, David Holmes wrote:

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
  instead of Hashtable


As you say in the bug report, now that the map is always accessed 
within synchronized code this serves no purpose. The map not only 
doesn't need to be concurrent, it doesn't even need to be thread-safe! 
So why not replace with a simple HashMap?


David


Hi Daniel,

...or, if you keep CHM, you might be able to use it for a benefit.

You say that retrieving a logger by name is the most frequent operation. 
If you can prove that moving a statement in addLocalLogger method that 
publishes the Logger via CHM to the end of synchronized block without 
hurting the logic that exists in-between and initializes the Logger 
instance, then you can implement the findLogger method in a way that 
almost never needs to enter synchronized block:


http://cr.openjdk.java.net/~plevart/jdk9-dev/LogManager.CHM/webrev.01/

Regards, Peter




https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

RE: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Jason Mehrens]

The snapshot enumeration is a welcomed change.  ConcurrentHashMap has legacy 
Hashtable methods so you can save a little bit by calling namedLoggers.keys() 
instead of wrapping the key set.


Jason


> Date: Thu, 26 Mar 2015 14:32:23 +0100
> From: daniel.fu...@oracle.com
> To: david.hol...@oracle.com; core-libs-dev@openjdk.java.net
> Subject: Re: RFR: 7113878: LogManager - namedLoggers should be 
> ConcurrentHashMap instead of Hashtable
>
> On 26/03/15 13:28, David Holmes wrote:
>> Hi Daniel,
>>
>> On 26/03/2015 10:08 PM, Daniel Fuchs wrote:
>>> Please find below a trivial fix for
>>>
>>>
>>> 7113878: LogManager - namedLoggers should be ConcurrentHashMap
>>> instead of Hashtable
>>
>> As you say in the bug report, now that the map is always accessed within
>> synchronized code this serves no purpose. The map not only doesn't need
>> to be concurrent, it doesn't even need to be thread-safe! So why not
>> replace with a simple HashMap?
>
> You found me out ;-) I should have mentioned it.
> The enumeration of keys returned from the map will be iterated
> outside of any synchronized block.
> ConcurrentHashMap makes this safe.
>
> best regards,
>
> -- daniel
>
>
>>
>> David
>>
>>> https://bugs.openjdk.java.net/browse/JDK-7113878
>>>
>>> http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00
>>>
>>> best regards,
>>>
>>> -- daniel
> 

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Daniel Fuchs]

On 26/03/15 13:28, David Holmes wrote:

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
  instead of Hashtable


As you say in the bug report, now that the map is always accessed within
synchronized code this serves no purpose. The map not only doesn't need
to be concurrent, it doesn't even need to be thread-safe! So why not
replace with a simple HashMap?


You found me out ;-) I should have mentioned it.
The enumeration of keys returned from the map will be iterated
outside of any synchronized block.
ConcurrentHashMap makes this safe.

best regards,

-- daniel




David


https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[David Holmes]

Hi Daniel,

On 26/03/2015 10:08 PM, Daniel Fuchs wrote:

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
  instead of Hashtable


As you say in the bug report, now that the map is always accessed within 
synchronized code this serves no purpose. The map not only doesn't need 
to be concurrent, it doesn't even need to be thread-safe! So why not 
replace with a simple HashMap?


David


https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel

Re: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Lance Andersen]

+1
On Mar 26, 2015, at 8:08 AM, Daniel Fuchs  wrote:

> Please find below a trivial fix for
> 
> 
> 7113878: LogManager - namedLoggers should be ConcurrentHashMap
> instead of Hashtable
> https://bugs.openjdk.java.net/browse/JDK-7113878
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00
> 
> best regards,
> 
> -- daniel



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Daniel Fuchs]

Please find below a trivial fix for


7113878: LogManager - namedLoggers should be ConcurrentHashMap
 instead of Hashtable
https://bugs.openjdk.java.net/browse/JDK-7113878

http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

best regards,

-- daniel