Re: Review request: JDK-8043277: Update jdk regression tests to extend the default security policy instead of override

[Staffan Larsen]

On 24 okt 2014, at 18:16, Mandy Chung mandy.ch...@oracle.com wrote:

 On 10/24/2014 6:33 AM, Staffan Larsen wrote:
 
 Since this is the first use of jtreg 4.1b10 features, this would also be 
 good time to tag the jdk test suite to require at least 4.1b10.
 
 This latest version of jtreg has support for checking which version of jtreg 
 the test suite requires. So you can add a line saying:
requiredVersion=4.1 b10
 to TEST.ROOT and jtreg will verify that its version number is higher than 
 “requiredVersion when it runs.
 
 It’s not until we move from b10 to b11 that this will actually be useful, 
 but it could be a good time to introduce it.
 
 Good point since now it depends on b10 feature.  Here is
 the patch.
 
 diff --git a/test/TEST.ROOT b/test/TEST.ROOT
 --- a/test/TEST.ROOT
 +++ b/test/TEST.ROOT
 @@ -12,3 +12,6 @@
  # Group definitions
 groups=TEST.groups [closed/TEST.groups]
 +
 +# Tests using jtreg 4.1 b10 features
 +requiredVersion=4.1 b10
 
 Mandy

Looks good!

Thanks,
/Staffan

 
 
 
 Thanks,
 /Staffan
 
 On 23 okt 2014, at 15:40, Alan Bateman alan.bate...@oracle.com wrote:
 
 On 23/10/2014 02:08, Mandy Chung wrote:
 jtreg policy option overrides the system security policy file and hence
 some existing test policy files have to duplicate the entries to grant
 permissions for JDK.
 
 This looks okay to me too. I think this will be the first use of a 
 jtreg4.1-b10 feature and maybe someone should send a note to jdk9-dev to 
 tell folks that they will need an up-to-date jtreg in order to test 
 jdk9/dev.
 
 -Alan
 
 

Re: RFR 8023173: FileDescriptor should respect append flag

[Alan Bateman]

On 25/10/2014 19:14, Ivan Gerasimov wrote:

Hello everyone!

I've changed the fix in order to address the concerns Alan had mentioned.

Now, both the Unix and Windows implementations of FileDescriptor class 
have the append flag.
First, it allows such querying the file descriptor in 
FileChannelImpl.position() that does not involve JNI calls.
Second, this flag is passed to the write functions as an argument, so 
there's no need to retrieve it from the native code.


The fix was built on all available platforms.
All the tests from io, nio pass.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/3/webrev/

Would you please help review this?

Thanks, this looks much better and cleaner than the previous iterations.

You can probably drop the setting of append in the no-arg FileDescriptor 
constructor. Also the final in FileChannelImpl.position isn't needed.


Otherwise this looks good to me and good to have this long standing 
corner case addressed.


-Alan

Re: RFR 8023173: FileDescriptor should respect append flag

[Ivan Gerasimov]

Thanks Alan!

I'll remove redundant initialization and final word before pushing.

Sincerely yours,
Ivan


On 27.10.2014 11:50, Alan Bateman wrote:

On 25/10/2014 19:14, Ivan Gerasimov wrote:

Hello everyone!

I've changed the fix in order to address the concerns Alan had 
mentioned.


Now, both the Unix and Windows implementations of FileDescriptor 
class have the append flag.
First, it allows such querying the file descriptor in 
FileChannelImpl.position() that does not involve JNI calls.
Second, this flag is passed to the write functions as an argument, so 
there's no need to retrieve it from the native code.


The fix was built on all available platforms.
All the tests from io, nio pass.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/3/webrev/

Would you please help review this?


Thanks, this looks much better and cleaner than the previous iterations.

You can probably drop the setting of append in the no-arg 
FileDescriptor constructor. Also the final in 
FileChannelImpl.position isn't needed.


Otherwise this looks good to me and good to have this long standing 
corner case addressed.


-Alan

Request for review/advice from langtools teamwas Re: Covariant overrides on the Buffer Hierarchy redux

[Paul Sandoz]

Hi,

Can someone from langtools kindly chime in with how to move this forward?

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

  http://cr.openjdk.java.net/~rwarburton/buffer-overrides-3/

  
http://cr.openjdk.java.net/~rwarburton/buffer-overrides-langtools-0/langtools.patch


On Oct 17, 2014, at 11:37 AM, Paul Sandoz paul.san...@oracle.com wrote:

 Hi,
 
 [Including compiler-dev, i am not on that list so please CC me if replying to 
 just that list]
 
 
...


 So Richard has a patch for that too:
 
   
 http://cr.openjdk.java.net/~rwarburton/buffer-overrides-langtools-0/langtools.patch
 
 This is required because in the bootstrap compilation phase a JDK without the 
 co-variant overrides can be used. So the current solution is to suppress the 
 warnings. Reviews from langtools gurus are very much appreciated.
 

?


 Ideally it would be nice to push all this under one issue, is that possible? 
 If not i will have to create another issue and of course push to langtools 
 before jdk.
 
 A internal CCC is also underway.
 

This has be approved, with the comment that @since 1.9 should be added to the 
doc of the new methods, which i have done in my copy of the patch.

Thanks,
Paul.

Re: [9] Review request : JDK-8059070: [TESTBUG] java/lang/invoke/LFCaching/LFMultiThreadCachingTest.java failed - timeout

[Konstantin Shefov]

Kindly reminder

On 23.10.2014 19:04, Paul Sandoz wrote:

On Oct 23, 2014, at 1:25 PM, Konstantin Shefov konstantin.she...@oracle.com 
wrote:

Gently reminder

On 17.10.2014 13:38, Konstantin Shefov wrote:

Hi,

I have updated the webrev:
http://cr.openjdk.java.net/~kshefov/8059070/webrev.01/


+1

Sorry for the delay,
Paul.

Re: Loading classes with many methods is very expensive

[Aleksey Shipilev]

On 10/27/2014 01:53 AM, Peter Levart wrote:
 As you might have noticed, the number of methods obtained from patched
 code differed from original code. I have investigated this and found
 that original code treats abstract class methods the same as abstract
 interface methods as far as multiple inheritance is concerned (it keeps
 them together in the returned array). So I fixed this and here's new
 webrev which behaves the same as original code:
 
 http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/

This seems to be a candidate fix for
https://bugs.openjdk.java.net/browse/JDK-8061950, right? If so, please
assign yourself there. It might be advisable to start the separate RFR
thread for a fix?

My comments for the first reading of the patch:

 * Why MethodList maintains the linked list? Doesn't O(n)
MethodList.add() lead to quadratic complexity again? Should we instead
use the ArrayListMethod for storing method synonyms?

 * Please convert some of the indexed loops into for-each loops for
better readability? E.g:

  for (int i = 0; i  intfcsMethods.length; i++) {
 for (Method m : intfcsMethods[i]) {

  ...to:

  for (Method[] ms : intfcsMethods) {
 for (Method m : ms) {

 * I would think the code would be more readable if MethodList.m was
having a more readable name, e.g. MethodList.base or MethodList.image or
MethodList.primary?

 * Formatting and logic in MethodList.consolidateWith gives me chills. I
think at very least introducing variables for m.getDeclaringClass() and
ml.m.getDeclaringClass() improve readability. Also, moving the comments
at the end of the line should improve readability and better highlight
the boolean expression you are spelling out. Below is the
straight-forward rewrite:

 MethodList consolidateWith(MethodList ml) {
 Method mineM = m;
 Method otherM = ml.m;
 Class? mine = mineM.getDeclaringClass();
 Class? other = otherM.getDeclaringClass();
 
 if (other == mine // 
 other is same method as mine
 || !Modifier.isAbstract(mineM.getModifiers()) // OR, 
 mine is non-abstract method...
  (other.isAssignableFrom(mine)  // 
 ...which overrides other
|| other.isInterface()  !mine.isInterface())) {  // 
 ...class method which wins over interface
 // just return
 return this;
 }
 if (!Modifier.isAbstract(otherM.getModifiers())   // 
 other is non-abstract method...
   (mine.isAssignableFrom(other) // 
 ...which overrides mine
  || mine.isInterface()  !other.isInterface())) {// 
 ...class method which wins over interface
 // knock m out and continue
 return (next == null) ? ml : next.consolidateWith(ml);
 }
 
 // else leave m in and continue
 next = (next == null) ? ml : next.consolidateWith(ml);
 return this;
 }


  * Should we use just methods.put() here?
  2851 MethodList ml0 = methods.putIfAbsent(ml, ml);

  * I wonder if the code would be simpler if we push the MapMethodList,
MethodList maintenance to some internal utility class? That would
probably simplify the loops in privateGetPublicMethods?

Thanks,
-Aleksey.

Re: Loading classes with many methods is very expensive

[Stanimir Simeonoff]

On Mon, Oct 27, 2014 at 1:23 PM, Aleksey Shipilev 
aleksey.shipi...@oracle.com wrote:

 On 10/27/2014 01:53 AM, Peter Levart wrote:
  As you might have noticed, the number of methods obtained from patched
  code differed from original code. I have investigated this and found
  that original code treats abstract class methods the same as abstract
  interface methods as far as multiple inheritance is concerned (it keeps
  them together in the returned array). So I fixed this and here's new
  webrev which behaves the same as original code:
 
  http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/

 This seems to be a candidate fix for
 https://bugs.openjdk.java.net/browse/JDK-8061950, right? If so, please
 assign yourself there. It might be advisable to start the separate RFR
 thread for a fix?

 My comments for the first reading of the patch:

  * Why MethodList maintains the linked list? Doesn't O(n)

MethodList.add() lead to quadratic complexity again? Should we instead
 use the ArrayListMethod for storing method synonyms?


But N would be the amount of exactly the same overridden methods from
superclasses and declared in interfaces. Usually there would be zero or
1-2. So the in-place linked list conserves memory, the extra indirection of
ArrayList for n=1 would also be slower. IMO, it's a well designed approach.
The only case where it'd actually matter is hundreds of superclasses each
overriding all of the their methods.

Stanimir


  * Please convert some of the indexed loops into for-each loops for
 better readability? E.g:

   for (int i = 0; i  intfcsMethods.length; i++) {
  for (Method m : intfcsMethods[i]) {

   ...to:

   for (Method[] ms : intfcsMethods) {
  for (Method m : ms) {

  * I would think the code would be more readable if MethodList.m was
 having a more readable name, e.g. MethodList.base or MethodList.image or
 MethodList.primary?

  * Formatting and logic in MethodList.consolidateWith gives me chills. I
 think at very least introducing variables for m.getDeclaringClass() and
 ml.m.getDeclaringClass() improve readability. Also, moving the comments
 at the end of the line should improve readability and better highlight
 the boolean expression you are spelling out. Below is the
 straight-forward rewrite:

  MethodList consolidateWith(MethodList ml) {
  Method mineM = m;
  Method otherM = ml.m;
  Class? mine = mineM.getDeclaringClass();
  Class? other = otherM.getDeclaringClass();
 
  if (other == mine //
 other is same method as mine
  || !Modifier.isAbstract(mineM.getModifiers()) //
 OR, mine is non-abstract method...
   (other.isAssignableFrom(mine)  //
 ...which overrides other
 || other.isInterface()  !mine.isInterface())) {  //
 ...class method which wins over interface
  // just return
  return this;
  }
  if (!Modifier.isAbstract(otherM.getModifiers())   //
 other is non-abstract method...
(mine.isAssignableFrom(other) //
 ...which overrides mine
   || mine.isInterface()  !other.isInterface())) {//
 ...class method which wins over interface
  // knock m out and continue
  return (next == null) ? ml : next.consolidateWith(ml);
  }
 
  // else leave m in and continue
  next = (next == null) ? ml : next.consolidateWith(ml);
  return this;
  }


   * Should we use just methods.put() here?
   2851 MethodList ml0 = methods.putIfAbsent(ml, ml);

   * I wonder if the code would be simpler if we push the MapMethodList,
 MethodList maintenance to some internal utility class? That would
 probably simplify the loops in privateGetPublicMethods?

 Thanks,
 -Aleksey.

Re: Loading classes with many methods is very expensive

[Peter Levart]

Hi Aleksey,

On 10/27/2014 12:23 PM, Aleksey Shipilev wrote:

On 10/27/2014 01:53 AM, Peter Levart wrote:

As you might have noticed, the number of methods obtained from patched
code differed from original code. I have investigated this and found
that original code treats abstract class methods the same as abstract
interface methods as far as multiple inheritance is concerned (it keeps
them together in the returned array). So I fixed this and here's new
webrev which behaves the same as original code:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/

This seems to be a candidate fix for
https://bugs.openjdk.java.net/browse/JDK-8061950, right? If so, please
assign yourself there. It might be advisable to start the separate RFR
thread for a fix?


That's exactly the issue I was looking for. I assigned it to myself.

Thanks for looking at the code...



My comments for the first reading of the patch:

  * Why MethodList maintains the linked list? Doesn't O(n)
MethodList.add() lead to quadratic complexity again? Should we instead
use the ArrayListMethod for storing method synonyms?


It's not quadratic, but more O(n*m) where n is the number of methods and 
m is the number of methods with same signature inherited via different 
paths from abstract class and/or (super)interfaces. This is typically 1, 
rarely 2, almost never 3 or more. For example:


abstract class A implements I, J, K {}

interface I {  void m(); }

interface J {  void m(); }

interface K {  void m(); }


Here the linked list with signature 'void m()' will contain 3 elements. 
When a particular method for a particular signature is found in a 
(super)interface, it has to be compared with potentially all methods 
having same signature and modifications performed while traversing are: 
do nothing, remove element, append element. So linked-list is perfect 
for that purpose. LinkedList.add() is only called when an abstract 
superclass brings multiple methods with same signature and all of them 
are inherited by abstract subclass - very very rarely is there more than 
1 method with same signature inherited from superclass.


I merged the functionality of the method-signature-key with the 
linked-list node, holding just a reference to Method object and a link 
to 'next' node. I think this is the most compact temporary datastructure 
representation for that purpose that leverages standard LinkedHashMap.




  * Please convert some of the indexed loops into for-each loops for
better readability? E.g:

   for (int i = 0; i  intfcsMethods.length; i++) {
  for (Method m : intfcsMethods[i]) {

   ...to:

   for (Method[] ms : intfcsMethods) {
  for (Method m : ms) {


Good catch. I'll do this.


  * I would think the code would be more readable if MethodList.m was
having a more readable name, e.g. MethodList.base or MethodList.image or
MethodList.primary?


Just MethodList.method will be ok - it's nothing special with this 
method. It just happens to be 1st in list of methods with same signature.


With introduction of local variables (later down), the conditions will 
be shortened in MethodList.consolidateWith() and short field name is not 
needed.





  * Formatting and logic in MethodList.consolidateWith gives me chills. I
think at very least introducing variables for m.getDeclaringClass() and
ml.m.getDeclaringClass() improve readability. Also, moving the comments
at the end of the line should improve readability and better highlight
the boolean expression you are spelling out. Below is the
straight-forward rewrite:


 MethodList consolidateWith(MethodList ml) {
 Method mineM = m;
 Method otherM = ml.m;
 Class? mine = mineM.getDeclaringClass();
 Class? other = otherM.getDeclaringClass();

 if (other == mine // other 
is same method as mine
 || !Modifier.isAbstract(mineM.getModifiers()) // OR, 
mine is non-abstract method...
  (other.isAssignableFrom(mine)  // 
...which overrides other
|| other.isInterface()  !mine.isInterface())) {  // 
...class method which wins over interface
 // just return
 return this;
 }
 if (!Modifier.isAbstract(otherM.getModifiers())   // other 
is non-abstract method...
   (mine.isAssignableFrom(other) // 
...which overrides mine
  || mine.isInterface()  !other.isInterface())) {// 
...class method which wins over interface
 // knock m out and continue
 return (next == null) ? ml : next.consolidateWith(ml);
 }

 // else leave m in and continue
 next = (next == null) ? ml : next.consolidateWith(ml);
 return this;
 }


Excellent. I'll take your formatting if you don't mind ;-)



   * Should we use just methods.put() here?
   2851 

Re: Loading classes with many methods is very expensive

[Joel Borggrén-Franck]

Hi Peter,

As always, thanks for doing this! It has been on my todolist for a while but 
never quite bubbling up to the top.

I don’t have time to look att his right now, but I expect to have some free 
time next week, but i have two short comments 

First, I have been thinking about moving MethodArray to its’s own top-level 
class, isn’t it about time?

Second I would expect testing for the missing cases you uncovered (good catch!).

I’ll try to get back to you asap.

cheers
/Joel


On 26 okt 2014, at 23:53, Peter Levart peter.lev...@gmail.com wrote:

 
 On 10/26/2014 09:25 PM, Peter Levart wrote:
 19657 classes loaded in 1.987373401 seconds.
 494141 methods obtained in 1.02493941 seconds.
 
 vs.
 
 19657 classes loaded in 2.084409717 seconds.
 494124 methods obtained in 0.915928578 seconds.
 
 Hi,
 
 As you might have noticed, the number of methods obtained from patched code 
 differed from original code. I have investigated this and found that original 
 code treats abstract class methods the same as abstract interface methods as 
 far as multiple inheritance is concerned (it keeps them together in the 
 returned array). So I fixed this and here's new webrev which behaves the same 
 as original code:
 
 http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/
 
 Comparing original vs. patched code still shows speed-up:
 
 Original:
 
 19657 classes loaded in 1.980493029 seconds.
 494141 methods obtained in 0.976318927 seconds.
 494141 methods obtained in 0.886504437 seconds.
 494141 methods obtained in 0.911153722 seconds.
 494141 methods obtained in 0.880550509 seconds.
 494141 methods obtained in 0.875526704 seconds.
 494141 methods obtained in 0.877258894 seconds.
 494141 methods obtained in 0.871794344 seconds.
 494141 methods obtained in 0.884159644 seconds.
 494141 methods obtained in 0.892648522 seconds.
 494141 methods obtained in 0.884581841 seconds.
 
 Patched:
 
 19657 classes loaded in 2.055697675 seconds.
 494141 methods obtained in 0.853922188 seconds.
 494141 methods obtained in 0.776203794 seconds.
 494141 methods obtained in 0.858774803 seconds.
 494141 methods obtained in 0.778178867 seconds.
 494141 methods obtained in 0.760043997 seconds.
 494141 methods obtained in 0.756352444 seconds.
 494141 methods obtained in 0.740826372 seconds.
 494141 methods obtained in 0.744264782 seconds.
 494141 methods obtained in 0.73805894 seconds.
 494141 methods obtained in 0.746852752 seconds.
 
 
 55 java/lang/reflect jtreg tests still pass. As they did before, which means 
 that we don't have a coverage for such cases. I'll see where I can add such a 
 case (EnumSet for example, which inherits from Set interface and 
 AbstractColection class via two different paths, so Set.size()/iterator() and 
 AbstractCollection.size()/iterator() are both returned from getMethods())...
 
 
 Regards, Peter
 

Re: Loading classes with many methods is very expensive

[Peter Levart]

Hi Joel,

Thanks for peeking into the code.

On 10/27/2014 02:45 PM, Joel Borggrén-Franck wrote:

Hi Peter,

As always, thanks for doing this! It has been on my todolist for a while but 
never quite bubbling up to the top.

I don’t have time to look att his right now, but I expect to have some free 
time next week, but i have two short comments

First, I have been thinking about moving MethodArray to its’s own top-level 
class, isn’t it about time?


Well, you're the second (first was Aleksey) to comment on factoring away 
the logic into a separate class. I'll do that then.




Second I would expect testing for the missing cases you uncovered (good catch!).


I plan to add a case or two to appropriate test. I'm thinking about 
test/java/lang/Class/getMethods/StarInheritance.java ...




I’ll try to get back to you asap.


Thank you,

Peter



cheers
/Joel


On 26 okt 2014, at 23:53, Peter Levart peter.lev...@gmail.com wrote:


On 10/26/2014 09:25 PM, Peter Levart wrote:

19657 classes loaded in 1.987373401 seconds.
494141 methods obtained in 1.02493941 seconds.

vs.

19657 classes loaded in 2.084409717 seconds.
494124 methods obtained in 0.915928578 seconds.

Hi,

As you might have noticed, the number of methods obtained from patched code 
differed from original code. I have investigated this and found that original 
code treats abstract class methods the same as abstract interface methods as 
far as multiple inheritance is concerned (it keeps them together in the 
returned array). So I fixed this and here's new webrev which behaves the same 
as original code:

http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods/webrev.02/

Comparing original vs. patched code still shows speed-up:

Original:

19657 classes loaded in 1.980493029 seconds.
494141 methods obtained in 0.976318927 seconds.
494141 methods obtained in 0.886504437 seconds.
494141 methods obtained in 0.911153722 seconds.
494141 methods obtained in 0.880550509 seconds.
494141 methods obtained in 0.875526704 seconds.
494141 methods obtained in 0.877258894 seconds.
494141 methods obtained in 0.871794344 seconds.
494141 methods obtained in 0.884159644 seconds.
494141 methods obtained in 0.892648522 seconds.
494141 methods obtained in 0.884581841 seconds.

Patched:

19657 classes loaded in 2.055697675 seconds.
494141 methods obtained in 0.853922188 seconds.
494141 methods obtained in 0.776203794 seconds.
494141 methods obtained in 0.858774803 seconds.
494141 methods obtained in 0.778178867 seconds.
494141 methods obtained in 0.760043997 seconds.
494141 methods obtained in 0.756352444 seconds.
494141 methods obtained in 0.740826372 seconds.
494141 methods obtained in 0.744264782 seconds.
494141 methods obtained in 0.73805894 seconds.
494141 methods obtained in 0.746852752 seconds.


55 java/lang/reflect jtreg tests still pass. As they did before, which means 
that we don't have a coverage for such cases. I'll see where I can add such a 
case (EnumSet for example, which inherits from Set interface and 
AbstractColection class via two different paths, so Set.size()/iterator() and 
AbstractCollection.size()/iterator() are both returned from getMethods())...


Regards, Peter

Re: Loading classes with many methods is very expensive

[Aleksey Shipilev]

Hi Peter,

On 10/27/2014 03:45 PM, Peter Levart wrote:
 I merged the functionality of the method-signature-key with the
 linked-list node, holding just a reference to Method object and a link
 to 'next' node. I think this is the most compact temporary datastructure
 representation for that purpose that leverages standard LinkedHashMap.

I see. I wonder when somebody come with a stress test using multiple
interfaces with the same signature method :) Seems very unlikely.


   * Formatting and logic in MethodList.consolidateWith gives me chills. I
 think at very least introducing variables for m.getDeclaringClass() and
 ml.m.getDeclaringClass() improve readability. Also, moving the comments
 at the end of the line should improve readability and better highlight
 the boolean expression you are spelling out. Below is the
 straight-forward rewrite:
 ...
 
 Excellent. I'll take your formatting if you don't mind ;-)

Sure. Check if I had captured it right first. I think we need to be more
strict with parentheses to avoid precedence overlooks:
  (A || B  C) should better be ((A || B)  C) or (A || (B  C))


* Should we use just methods.put() here?
2851 MethodList ml0 = methods.putIfAbsent(ml, ml);
 
 It really doesn't matter as the exception is thrown if (ml0 != null) and
 LinkedHashMap is forgotten...
 
 if (ml0 == null), put and putIfAbsent are equivalent. I used putIfAbsent
 to make the 3 loops (declared methods, superclass methods,
 (super)interface methods) more uniform.

Okay.



* I wonder if the code would be simpler if we push the MapMethodList,
 MethodList maintenance to some internal utility class? That would
 probably simplify the loops in privateGetPublicMethods?
 
 I thought MethodList.add()/consolidateWith() were close to those utility
 methods. I could extract the logic in each of 3 loops to 3 void methods
 taking 'methods' Map and a Method instance, but I don't want to pollute
 the method namespace in j.l.Class any more than necessary and making a
 special inner class for that is an overkill. Previously the holder for
 such methods was MethodArray, but it's gone now. I could subclass
 LinkedHashMap if I wanted to avoid an unnecessary indirection, but
 that's not a good practice. It's not that bad as it is - 38 lines of
 code for 3 loops with comments and spacing. I could improve comments
 though...

Yes. I had to read the entire thing a couple of times to understand how
this works. Containing most of the logic into MethodList seems like a
way to improve this. Not sure though.


Thanks,
-Aleksey.

Re: Lower overhead String encoding/decoding

[Xueming Shen]

On 10/26/2014 02:10 PM, Richard Warburton wrote:




The getBytes(ByteBuffer, Charset) method needs a bit more javadoc. You
should be able to borrow from text from the CharsetEncoder#encode methods
to help with that.


I've updated the Javadoc with more information about the encoding and made
these changes. I'm not sure if there's anything else that's missing in this
case.



Hi Richard,

You have ...reflect the characters read and the byte written... for the 
getBtyes(ByteBuffer),
the characters read and should be deleted?

I'm not that sure if it is necessary to document it clearly that there would be 
no dangling
bytes in case of encoding/getBytes(), when there is no enough room/space to 
encode a
full char...

-Sherman

RFR 8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable

[Lance Andersen]

Hi all,

Need a reviewer for  the RowSetMetaDataImpl tests which also includes a fix to 
isDefinitelyWritable so that it validates the column index ranges

Included are some minor housekeeping changes to remove redundant classes

The webrev can be found at  
http://cr.openjdk.java.net/~lancea/8062198/webrev.00/

Best
Lance


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: Loading classes with many methods is very expensive

[Martin Buchholz]

I'm having trouble keeping up with this thread I started, but I did update
the test/benchmark
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/

I added a test *LoadAllClassesAndMethods *that does what its name says.
Although originally written as just a benchmark, it also checks en
passant that all classes in all JRE jar files can be loaded.
Interestingly, openjdk9 passes this test, but proprietary Oracle JDK fails
it.  I think this is a useful packaging-level test of the entire JDK as
well.

java.lang.NoClassDefFoundError: Failed to load
jfxrt.jar(com/sun/deploy/uitoolkit/impl/fx/FXApplet2Adapter$1.class)
at
LoadAllClassesAndMethods.loadAllExtensionClasses(LoadAllClassesAndMethods.java:178)
at
LoadAllClassesAndMethods.methodsByClass(LoadAllClassesAndMethods.java:198)
at LoadAllClassesAndMethods.main(LoadAllClassesAndMethods.java:65)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at com.sun.javatest.regtest.MainWrapper$MainThread.run(MainWrapper.java:94)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NoClassDefFoundError:
com/sun/deploy/uitoolkit/Applet2Adapter
at java.lang.ClassLoader.defineClass1(Native Method)

Jeremy is independently working on yet another performance problem exposed
by *LoadAllClassesAndMethods*


On Mon, Oct 27, 2014 at 8:48 AM, Aleksey Shipilev 
aleksey.shipi...@oracle.com wrote:

 Hi Peter,

 On 10/27/2014 03:45 PM, Peter Levart wrote:
  I merged the functionality of the method-signature-key with the
  linked-list node, holding just a reference to Method object and a link
  to 'next' node. I think this is the most compact temporary datastructure
  representation for that purpose that leverages standard LinkedHashMap.

 I see. I wonder when somebody come with a stress test using multiple
 interfaces with the same signature method :) Seems very unlikely.


* Formatting and logic in MethodList.consolidateWith gives me chills.
 I
  think at very least introducing variables for m.getDeclaringClass() and
  ml.m.getDeclaringClass() improve readability. Also, moving the comments
  at the end of the line should improve readability and better highlight
  the boolean expression you are spelling out. Below is the
  straight-forward rewrite:
  ...
 
  Excellent. I'll take your formatting if you don't mind ;-)

 Sure. Check if I had captured it right first. I think we need to be more
 strict with parentheses to avoid precedence overlooks:
   (A || B  C) should better be ((A || B)  C) or (A || (B  C))


 * Should we use just methods.put() here?
 2851 MethodList ml0 = methods.putIfAbsent(ml, ml);
 
  It really doesn't matter as the exception is thrown if (ml0 != null) and
  LinkedHashMap is forgotten...
 
  if (ml0 == null), put and putIfAbsent are equivalent. I used putIfAbsent
  to make the 3 loops (declared methods, superclass methods,
  (super)interface methods) more uniform.

 Okay.


 
 * I wonder if the code would be simpler if we push the
 MapMethodList,
  MethodList maintenance to some internal utility class? That would
  probably simplify the loops in privateGetPublicMethods?
 
  I thought MethodList.add()/consolidateWith() were close to those utility
  methods. I could extract the logic in each of 3 loops to 3 void methods
  taking 'methods' Map and a Method instance, but I don't want to pollute
  the method namespace in j.l.Class any more than necessary and making a
  special inner class for that is an overkill. Previously the holder for
  such methods was MethodArray, but it's gone now. I could subclass
  LinkedHashMap if I wanted to avoid an unnecessary indirection, but
  that's not a good practice. It's not that bad as it is - 38 lines of
  code for 3 loops with comments and spacing. I could improve comments
  though...

 Yes. I had to read the entire thing a couple of times to understand how
 this works. Containing most of the logic into MethodList seems like a
 way to improve this. Not sure though.


 Thanks,
 -Aleksey.

Re: RFR: 8062194: java.util.jar.Attributes should use insertion-ordered iteration

[Martin Buchholz]

[+core-libs-dev oops I forgot to cc: the first time...]

On Mon, Oct 27, 2014 at 12:02 PM, Xueming Shen xueming.s...@oracle.com
wrote:

 On 10/27/2014 11:33 AM, Martin Buchholz wrote:

 Hello Xueming, Alan,

 I'd like you to do a code review.

 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/
 Attributes-iteration-order/ http://cr.openjdk.java.net/%
 7Emartin/webrevs/openjdk9/Attributes-iteration-order/
 https://bugs.openjdk.java.net/browse/JDK-8062194


 The change looks fine. But guess we might have to go through the CCC for
 this one,
 given the nature of its incompatibility?


Yes - technically, this is a small incompatibility.  (But Attribute
iteration order has changed many times)


 Btw, is there any noticeable performance concern of switching from
 hashmap to linkedhashmap?
 Guess, we might have use scenario that lots of attributes is being access
 when lots of jar get
 opened the same time...


LinkedHashMap uses a little more cpu and memory.  The cost is small
enough that some people have suggested simply replacing HashMap's
implementation with that of LinkedHashMap, and that is not totally crazy,
but we're not going that far.  Attributes are unlikely to contain many
elements or to be long-lived.

Re: RFR 8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable

[huizhe wang]

Hi Lance,

Does  test37 needs to loop through all of the columns? There doesn't 
seem to be a differentiator or different factor between the columns, e.g 
if one column is set to auto_increment and another not, the results 
would be different. Also in the test, some of the setters seem to be 
invalid, for example, whether the column is auto_increment or not shall 
affect setters such as setPrecision, setScale, setNullable, isReadOnly, 
isWritable and etc., shouldn't it?


-Joe

On 10/27/2014 11:06 AM, Lance Andersen wrote:

Hi all,

Need a reviewer for  the RowSetMetaDataImpl tests which also includes a fix to 
isDefinitelyWritable so that it validates the column index ranges

Included are some minor housekeeping changes to remove redundant classes

The webrev can be found at  
http://cr.openjdk.java.net/~lancea/8062198/webrev.00/

Best
Lance


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 8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable

[Lance Andersen]

Hi Joe,

Thank you for taking time to help review.
On Oct 27, 2014, at 3:27 PM, huizhe wang huizhe.w...@oracle.com wrote:

 Hi Lance,
 
 Does  test37 needs to loop through all of the columns?

Wanted a test which made sure that for the number of columns in the metadata 
that the fields can be set.  Does it have to, not really, but it wanted to 
sanity check that they could
 There doesn't seem to be a differentiator or different factor between the 
 columns, e.g if one column is set to auto_increment and another not, the 
 results would be different. Also in the test, some of the setters seem to be 
 invalid, for example, whether the column is auto_increment or not shall 
 affect setters such as setPrecision, setScale, setNullable, isReadOnly, 
 isWritable and etc., shouldn't it?

For this abstract class it is just a matter of validating the methods can be 
set for the various columns.

in an implementation of a RowSet, the data associated with a given column will 
have more actual meaning based on the column definition.  So yes the values 
being set  are  somewhat artificial but that is OK in this instance.

Also different databases for example can allow an auto-incrementable column to 
be writable, others do not so your milage varies as to which fields can and 
cannot have coresponding values.

Best,
Lance
 
 -Joe
 
 On 10/27/2014 11:06 AM, Lance Andersen wrote:
 Hi all,
 
 Need a reviewer for  the RowSetMetaDataImpl tests which also includes a fix 
 to isDefinitelyWritable so that it validates the column index ranges
 
 Included are some minor housekeeping changes to remove redundant classes
 
 The webrev can be found at  
 http://cr.openjdk.java.net/~lancea/8062198/webrev.00/
 
 Best
 Lance
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com
 
 
 
 



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: Loading classes with many methods is very expensive

[Aleksey Shipilev]

On 27.10.2014 22:01, Martin Buchholz wrote:
 I'm having trouble keeping up with this thread I started, but I did
 update the test/benchmark
 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/
 http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk9/Class.getMethods-benchmark/
 
 I added a test *LoadAllClassesAndMethods *that does what its name says. 
 Although originally written as just a benchmark, 

In our recent Nashorn classloading work, we did this benchmark:
 http://cr.openjdk.java.net/~shade/8053904/ClassLoaderBenchmark.java

...which, I think can be easily exploded to call getMethods() as well.


 Jeremy is independently working on yet another performance problem
 exposed by *LoadAllClassesAndMethods*

Does Jeremy have a reportable outline? Submit the bug to make this
thread short :)


Thanks,
-Aleksey.

Re: RFR 8062198: Add RowSetMetaDataImpl Tests and add column range validation to isdefinitlyWritable

[huizhe wang]

Hi Lance,

Thanks for the explanation, so I learned a bit about these APIs :-) I 
had thought RowSetMetaDataImpl is The implementation of a RowSet.


The changes look good to me.

-Joe

On 10/27/2014 12:37 PM, Lance Andersen wrote:

Hi Joe,

Thank you for taking time to help review.
On Oct 27, 2014, at 3:27 PM, huizhe wang huizhe.w...@oracle.com 
mailto:huizhe.w...@oracle.com wrote:



Hi Lance,

Does  test37 needs to loop through all of the columns?


Wanted a test which made sure that for the number of columns in the 
metadata that the fields can be set.  Does it have to, not really, but 
it wanted to sanity check that they could
There doesn't seem to be a differentiator or different factor between 
the columns, e.g if one column is set to auto_increment and another 
not, the results would be different. Also in the test, some of the 
setters seem to be invalid, for example, whether the column is 
auto_increment or not shall affect setters such as setPrecision, 
setScale, setNullable, isReadOnly, isWritable and etc., shouldn't it?


For this abstract class it is just a matter of validating the methods 
can be set for the various columns.


in an implementation of a RowSet, the data associated with a given 
column will have more actual meaning based on the column definition. 
 So yes the values being set  are  somewhat artificial but that is OK 
in this instance.


Also different databases for example can allow an auto-incrementable 
column to be writable, others do not so your milage varies as to which 
fields can and cannot have coresponding values.


Best,
Lance


-Joe

On 10/27/2014 11:06 AM, Lance Andersen wrote:

Hi all,

Need a reviewer for  the RowSetMetaDataImpl tests which also 
includes a fix to isDefinitelyWritable so that it validates the 
column index ranges


Included are some minor housekeeping changes to remove redundant classes

The webrev can be found at 
http://cr.openjdk.java.net/~lancea/8062198/webrev.00/ 
http://cr.openjdk.java.net/%7Elancea/8062198/webrev.00/


Best
Lance


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







http://oracle.com/us/design/oracle-email-sig-198324.gif
http://oracle.com/us/design/oracle-email-sig-198324.gifhttp://oracle.com/us/design/oracle-email-sig-198324.gif
http://oracle.com/us/design/oracle-email-sig-198324.gifLance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

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

RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Ioi Lam]

Hi David, I have update the latest webrev at:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/

and fixed the int cache[] style you mentioned.

This version also contains the JDK test case that Mandy requested:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html

Thanks
- Ioi


On 10/26/14, 4:27 PM, David Holmes wrote:

Style nit: all the

int cache[]

should be

int[] cache

Also not clear if 8061651-lookup-index-open-v2 is latest webrev ??

Thanks,
David

On 25/10/2014 9:38 AM, Ioi Lam wrote:

Hi Mandy,

Thanks for the review. please see comments in-line

On 10/24/14, 2:33 PM, Mandy Chung wrote:


On 10/23/2014 9:34 PM, Ioi Lam wrote:

Hi Mandy,

Thanks for the review. I have updated the patch:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v2/



Thanks for removing the LoaderType enum.  Two questions -
do you need the new hasLookupCacheLoader variable?  Should
lookupCAcheURLs be always be non-null if it has the lookup
cache?

I removed hasLookupCacheLoader and changed the condition check to
(lookupCacheURLs != null)


Most methods referencinglookupCacheURLs and lookupCacheLoader
are synchronized except initLookupCache and knowToNotExist.
Should they?  Or volatile?


I changed both to synchronized.

On 10/21/14, 12:56 PM, Mandy Chung wrote:


line 398: what happens if loaders.size()  maxindex?  Shouldn't
   it return null?

In this case, all the loaders that's needed by the cache[] have been
opened. So we should return cache[].


I forget about that, sorry.I'm thinking how to make it more
obvious to those who doesn't know much of the details.  Every time I
read this and I need to reload what I know about and miss something.


I changed the code to this. What do you think?


It seems to me that it would help if you refactor and add a method
ensureLoadersInited that will make it clear what it attempts to do.
The side effect invalidating the cache can be checked after it's
called and no need to worry about lookupCacheEnabled must be checked
in any order.  Something like this
  if (cache != null  cache.length  0) {
 int maxindex = cache[cache.length - 1];
 ensureLoaderOpened(maxindex);
 if (loaders.get(maxindex) == null || !lookupCacheEnabled) {
 if (DEBUG_LOOKUP_CACHE) {
System.out.println(Expanded loaders FAILED  +
loaders.size() +  for maxindex= +
maxindex);
 }
 return null;
 }
 ...
  }
  return cache;

I changed the code to

private synchronized int[] getLookupCache(String name) {
 if (lookupCacheURLs == null || !lookupCacheEnabled) {
 return null;
 }

 int cache[] = getLookupCacheForClassLoader(lookupCacheLoader,
name);
 if (cache != null  cache.length  0) {
 int maxindex = cache[cache.length - 1]; // cache[] is
strictly ascending.
 if (!ensureLoaderOpened(maxindex)) {
 if (DEBUG_LOOKUP_CACHE) {
 System.out.println(Expanded loaders FAILED  +
loaders.size() +  for
maxindex= + maxindex);
 }
 return null;
 }
 }

 return cache;
 }

 private boolean ensureLoaderOpened(int index) {
 if (loaders.size() = index) {
 // Open all Loaders up to, and including, index
 if (getLoader(index) == null) {
 return false;
 }
 if (!lookupCacheEnabled) {
 // cache was invalidated as the result of the above 
call.

 return false;
 }
 if (DEBUG_LOOKUP_CACHE) {
 System.out.println(Expanded loaders  + 
loaders.size() +

 to index= + index);
 }
 }
 return true;
 }


The code is getting further complicated with this lookup cache and
bear with me for being picky.


if (loaders.size() = maxindex) {
boolean failed = false;

// Call getNextLoader() with a null cache to open all
// Loaders up to, and including, maxindex.
if (getNextLoader(null, maxindex) == null) {


Can this call getLoader(maxindex)?  They are essentially the same.
I think getLoader(maxindex) makes it explicit that it forces to
open the loader.

Changed.


Mandy

Re: Loading classes with many methods is very expensive

[Jeremy Manson]

Jeremy has already filed a bug (8062116), and is just finishing up the
patch (I jumped the gun a bit by attaching it to the bug - it has a few
bugs in it).  I'll probably have something in reasonable shape to review
tomorrow (I've submitted it internally, and I want to let it spend a day or
so having our test infrastructure throw things at it).

It's actually a JVMTI performance regression.  The data structure that is
being used to store jmethodids isn't great.  I've refactored it a bit to
improve its performance, but it should really be a closed hash table.
There isn't a handy one in HS, so I just made it O(1) for writes.  It's
still O(n) for reads, though, which is nasty.

We can have the real conversation when I post the RFR, though.

Jeremy

On Mon, Oct 27, 2014 at 12:45 PM, Aleksey Shipilev 
aleksey.shipi...@oracle.com wrote:

 On 27.10.2014 22:01, Martin Buchholz wrote:
  I'm having trouble keeping up with this thread I started, but I did
  update the test/benchmark
 
 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/
  
 http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk9/Class.getMethods-benchmark/
 
 
  I added a test *LoadAllClassesAndMethods *that does what its name says.
  Although originally written as just a benchmark,

 In our recent Nashorn classloading work, we did this benchmark:
  http://cr.openjdk.java.net/~shade/8053904/ClassLoaderBenchmark.java

 ...which, I think can be easily exploded to call getMethods() as well.


  Jeremy is independently working on yet another performance problem
  exposed by *LoadAllClassesAndMethods*

 Does Jeremy have a reportable outline? Submit the bug to make this
 thread short :)


 Thanks,
 -Aleksey.

RFR (xs) 8062233: add java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java

[Stuart Marks]

Hi all,

Quick addition to the problem list. This test has been failing intermittently 
for a while, but for some reason it seems to be failing a bit more often 
recently. Please review the patch below.


Thanks,

s'marks



# HG changeset patch
# User smarks
# Date 1414457168 25200
#  Mon Oct 27 17:46:08 2014 -0700
# Node ID ac55626c516fbc4bdf218d22837fc9a74cd2f0b2
# Parent  67a92afb97bc7110edaf49a03c1cc51e04463bd4
8062233: add java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java 
to problem list

Reviewed-by: XXX

diff -r 67a92afb97bc -r ac55626c516f test/ProblemList.txt
--- a/test/ProblemList.txt  Mon Oct 27 13:45:39 2014 -0700
+++ b/test/ProblemList.txt  Mon Oct 27 17:46:08 2014 -0700
@@ -200,6 +200,9 @@

 # jdk_rmi

+# 7140992
+java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java  generic-all
+
 # 7146541
 java/rmi/transport/rapidExportUnexport/RapidExportUnexport.java
linux-all

Re: RFR (xs) 8062233: add java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java

[Joe Darcy]

Thumbs up; cheers,

-Joe

On 10/27/2014 5:52 PM, Stuart Marks wrote:

Hi all,

Quick addition to the problem list. This test has been failing 
intermittently for a while, but for some reason it seems to be failing 
a bit more often recently. Please review the patch below.


Thanks,

s'marks



# HG changeset patch
# User smarks
# Date 1414457168 25200
#  Mon Oct 27 17:46:08 2014 -0700
# Node ID ac55626c516fbc4bdf218d22837fc9a74cd2f0b2
# Parent  67a92afb97bc7110edaf49a03c1cc51e04463bd4
8062233: add 
java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java to 
problem list

Reviewed-by: XXX

diff -r 67a92afb97bc -r ac55626c516f test/ProblemList.txt
--- a/test/ProblemList.txtMon Oct 27 13:45:39 2014 -0700
+++ b/test/ProblemList.txtMon Oct 27 17:46:08 2014 -0700
@@ -200,6 +200,9 @@

 # jdk_rmi

+# 7140992
+java/rmi/server/Unreferenced/finiteGCLatency/FiniteGCLatency.java 
generic-all

+
 # 7146541
 java/rmi/transport/rapidExportUnexport/RapidExportUnexport.java 
linux-all

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Mandy Chung]

On 10/27/2014 3:32 PM, Ioi Lam wrote:

Hi David, I have update the latest webrev at:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/



The update looks good.  Thanks.


This version also contains the JDK test case that Mandy requested:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html 





What I request to add is a test setting the system property 
(-Dsun.cds.enableSharedLookupCache=true) and continue to load class A 
and B.  Removing line 44-58 should do it and also no need to set 
-Dfoo.foo.bar.


It'd be good if you run this test and turn on the debug traces to make 
sure that the application class loader and ext class loader will start 
up with the lookup cache enabled and make up call to the VM.  As it 
doesn't have the app cds archive, it will invalidate the cache right 
away and continue the class lookup with null cache array.


Mandy

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Jiangli Zhou]

Hi Ioi,

I have a question for following code in AppClassLoader.loadClass(). If a 
class is 'known to not exist' for the AppClassLoader (not in the 
AppClassLoader and parent classloaders' shared lookup cache), it seems 
findLoadedClass() would only find classes that's dynamically loaded by 
the parent loaders. The AppClassLoader would never try to load the 
class. Is the AppClassLoader's lookup cache guaranteed to have all the 
classes in it's path?


 315 if (ucp.knownToNotExist(name)) {
 316 // The class of the given name is not found in the parent
 317 // class loader as well as its local URLClassPath.
 318 // Check if this class has already been defined 
dynamically;
 319 // if so, return the loaded class; otherwise, skip the 
parent
 320 // delegation and findClass.
 321 synchronized (getClassLoadingLock(name)) {
 322 Class? c = findLoadedClass(name);
 323 if (c != null) {
 324 return c;
 325 }
 326 }
 327 throw new ClassNotFoundException(name);
 328 }


Thanks,
Jiangli

On 10/27/2014 3:32 PM, Ioi Lam wrote:

Hi David, I have update the latest webrev at:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/

and fixed the int cache[] style you mentioned.

This version also contains the JDK test case that Mandy requested:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html 



Thanks
- Ioi

Re: [9] [8u40] RFR (M): 8059877: GWT branch frequencies pollution due to LF sharing

[John Rose]

On Oct 15, 2014, at 8:21 AM, Vladimir Ivanov vladimir.x.iva...@oracle.com 
wrote:

 Updated version:
  http://cr.openjdk.java.net/~vlivanov/8059877/webrev.01/


The algorithm looks fine, as long as the count is small.  (Otherwise we might 
want to spend effort recompiling the DontInline LF.

Call it CountingWrapper, since that's what it does.  (Count down vs. count up 
doesn't matter much.)

Then you can call the states counting and not counting.  The pre-action is 
present only in the counting state.
The aspect of inlining or blocking is then focused down to a detail of the LF 
flavor.

You call DelegatingMethodHandle.makeReinvokerForm in three places, two of which 
are identical calls.  I suggest refactoring so as to call it in two places.

(Nit: As a matter of style, the default value of a boolean flag should be usual 
one, in typical cases.  By that reasoning, LF.forceInline should be 
LF.dontInline; the objection to this is that it is anti-style to have a 
negative word in a boolean name.)

You could create and cache *both* reinvoker forms when the MH wrapper is 
created, so that the non-counting, inline-enabled LF is created more eagerly 
during warmup.  It might make for a smoother warmup.  (Maybe.  Maybe not.)

Thanks!
— John

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Ioi Lam]

On 10/27/14, 7:04 PM, Mandy Chung wrote:


On 10/27/2014 3:32 PM, Ioi Lam wrote:

Hi David, I have update the latest webrev at:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/



The update looks good.  Thanks.


This version also contains the JDK test case that Mandy requested:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html 





What I request to add is a test setting the system property 
(-Dsun.cds.enableSharedLookupCache=true) and continue to load class A 
and B.  Removing line 44-58 should do it and also no need to set 
-Dfoo.foo.bar.


Do you mean change the test to call 
System.setProperty(sun.cds.enableSharedLookupCache, true)?


But we know that the property is checked only once, before any app 
classes are loaded. So calling System.setProperty in an application 
class won't test anything.
It'd be good if you run this test and turn on the debug traces to make 
sure that the application class loader and ext class loader will start 
up with the lookup cache enabled and make up call to the VM.  As it 
doesn't have the app cds archive, it will invalidate the cache right 
away and continue the class lookup with null cache array.
In the latest code, if CDS is not available, lookupCacheEnabled will be 
set to false inside the static initializer of URLClassPath:


private static volatile boolean lookupCacheEnabled
= 
true.equals(VM.getSavedProperty(sun.cds.enableSharedLookupCache));


later, when the boot/ext/app loaders call into here:

synchronized void initLookupCache(ClassLoader loader) {
if ((lookupCacheURLs = getLookupCacheURLs(loader)) != null) {
lookupCacheLoader = loader;
} else {
// This JVM instance does not support lookup cache.
disableAllLookupCaches();
}
}

their lookupCacheURLs[] fields will all be set to null. As a result, 
getLookupCacheForClassLoader and knownToNotExist0 will never be called.


I can add a DEBUG_LOOKUP_CACHE trace inside disableAllLookupCaches to 
print lookup cache disabled, and check for that in the test. Is this OK?


Thanks
- Ioi

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Mandy Chung]

On 10/27/2014 9:38 PM, Ioi Lam wrote:


What I request to add is a test setting the system property 
(-Dsun.cds.enableSharedLookupCache=true) and continue to load class A 
and B.  Removing line 44-58 should do it and also no need to set 
-Dfoo.foo.bar.


Do you mean change the test to call 
System.setProperty(sun.cds.enableSharedLookupCache, true)?


But we know that the property is checked only once, before any app 
classes are loaded. So calling System.setProperty in an application 
class won't test anything.


No, set it in the command-line (i.e. @run) is fine.

Removing line 44-58  should do it.

It'd be good if you run this test and turn on the debug traces to 
make sure that the application class loader and ext class loader will 
start up with the lookup cache enabled and make up call to the VM.  
As it doesn't have the app cds archive, it will invalidate the cache 
right away and continue the class lookup with null cache array.
In the latest code, if CDS is not available, lookupCacheEnabled will 
be set to false inside the static initializer of URLClassPath:


private static volatile boolean lookupCacheEnabled
= 
true.equals(VM.getSavedProperty(sun.cds.enableSharedLookupCache));


later, when the boot/ext/app loaders call into here:

synchronized void initLookupCache(ClassLoader loader) {
if ((lookupCacheURLs = getLookupCacheURLs(loader)) != null) {
lookupCacheLoader = loader;
} else {
// This JVM instance does not support lookup cache.
disableAllLookupCaches();
}
}

their lookupCacheURLs[] fields will all be set to null. As a result, 
getLookupCacheForClassLoader and knownToNotExist0 will never be called.




It will call getLookupCacheURLs.  It's just a sanity test and it's fine 
to call one but not all three.


I can add a DEBUG_LOOKUP_CACHE trace inside disableAllLookupCaches to 
print lookup cache disabled, and check for that in the test. Is this 
OK?


As long as A.test and B.test are invoked and finishes, it should be 
adequate.


Mandy

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Ioi Lam]

On 10/27/14, 7:52 PM, Jiangli Zhou wrote:

Hi Ioi,

I have a question for following code in AppClassLoader.loadClass(). If 
a class is 'known to not exist' for the AppClassLoader (not in the 
AppClassLoader and parent classloaders' shared lookup cache), it seems 
findLoadedClass() would only find classes that's dynamically loaded by 
the parent loaders. The AppClassLoader would never try to load the 
class. Is the AppClassLoader's lookup cache guaranteed to have all the 
classes in it's path?


 315 if (ucp.knownToNotExist(name)) {
 316 // The class of the given name is not found in 
the parent

 317 // class loader as well as its local URLClassPath.
 318 // Check if this class has already been defined 
dynamically;
 319 // if so, return the loaded class; otherwise, 
skip the parent

 320 // delegation and findClass.
 321 synchronized (getClassLoadingLock(name)) {
 322 Class? c = findLoadedClass(name);
 323 if (c != null) {
 324 return c;
 325 }
 326 }
 327 throw new ClassNotFoundException(name);
 328 }
The reason is to make the behavior consistent with 
java.lang.ClassLoader.loadClass():


protected Class? loadClass(String name, boolean resolve)
throws ClassNotFoundException
{
synchronized (getClassLoadingLock(name)) {
// First, check if the class has already been loaded
Class? c = findLoadedClass(name);


If name is not in any of the JAR files but was dynamically defined 
(using ClassLoader.defineClass, etc), then AppClassLoader.loadClass() 
should return the class without throwing ClassNotFoundException.


Thanks
- Ioi


Thanks,
Jiangli

On 10/27/2014 3:32 PM, Ioi Lam wrote:

Hi David, I have update the latest webrev at:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/

and fixed the int cache[] style you mentioned.

This version also contains the JDK test case that Mandy requested:

http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v3/jdk/test/sun/misc/URLClassPath/EnableLookupCache.java.html 



Thanks
- Ioi

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Ioi Lam]

On 10/27/14, 9:52 PM, Mandy Chung wrote:


On 10/27/2014 9:38 PM, Ioi Lam wrote:


What I request to add is a test setting the system property 
(-Dsun.cds.enableSharedLookupCache=true) and continue to load class 
A and B.  Removing line 44-58 should do it and also no need to set 
-Dfoo.foo.bar.


Do you mean change the test to call 
System.setProperty(sun.cds.enableSharedLookupCache, true)?


But we know that the property is checked only once, before any app 
classes are loaded. So calling System.setProperty in an application 
class won't test anything.


No, set it in the command-line (i.e. @run) is fine.

Removing line 44-58  should do it.


I will remove the check from line 44 - 48.

I want to keep the -Dfoo.foo.bar=xyz and the corresponding check to make 
sure that the JTREG framework is working as intended. Otherwise JTREG 
could have skipped the -Dsun.cds.enableSharedLookupCache=true and the 
test will still report PASS even though the intended action was not 
performed.


It'd be good if you run this test and turn on the debug traces to 
make sure that the application class loader and ext class loader 
will start up with the lookup cache enabled and make up call to the 
VM.  As it doesn't have the app cds archive, it will invalidate the 
cache right away and continue the class lookup with null cache array.
In the latest code, if CDS is not available, lookupCacheEnabled will 
be set to false inside the static initializer of URLClassPath:


private static volatile boolean lookupCacheEnabled
= 
true.equals(VM.getSavedProperty(sun.cds.enableSharedLookupCache));


later, when the boot/ext/app loaders call into here:

synchronized void initLookupCache(ClassLoader loader) {
if ((lookupCacheURLs = getLookupCacheURLs(loader)) != null) {
lookupCacheLoader = loader;
} else {
// This JVM instance does not support lookup cache.
disableAllLookupCaches();
}
}

their lookupCacheURLs[] fields will all be set to null. As a result, 
getLookupCacheForClassLoader and knownToNotExist0 will never be called.




It will call getLookupCacheURLs.  It's just a sanity test and it's 
fine to call one but not all three.


I can add a DEBUG_LOOKUP_CACHE trace inside disableAllLookupCaches to 
print lookup cache disabled, and check for that in the test. Is 
this OK?


As long as A.test and B.test are invoked and finishes, it should be 
adequate.


Mandy

Re: RFR (M) 8061651 - Interface to the Lookup Index Cache to improve URLClassPath search time (round 3)

[Mandy Chung]

On 10/27/2014 10:03 PM, Ioi Lam wrote:

I will remove the check from line 44 - 48.

I want to keep the -Dfoo.foo.bar=xyz and the corresponding check to 
make sure that the JTREG framework is working as intended. Otherwise 
JTREG could have skipped the -Dsun.cds.enableSharedLookupCache=true 
and the test will still report PASS even though the intended action 
was not performed. 


Now I see what the test is trying to verify: 
sun.cds.enableSharedLookupCache should be removed from the system 
properties as it's only an interface with the library.


There is no harm in having the test as is while I would say 
-Dfoo.foo.bar=xyz is not really needed; otherwise other tests may fail 
if jtreg does something wrong.


Approved what you have in v3.

thanks
Mandy