Re: RFR: 8042906: Remove use of reflection in ORB

[Chris Hegarty]

On 13 May 2014, at 19:53, Seán Coffey  wrote:

> That's some nice code reduction Paul. Thanks.
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8042906.v3/webrev/
> 
> I'll push these changes later unless I hear to the contrary.

Push it quick!

-Chris.

> 
> regards,
> Sean.
> 
> On 13/05/2014 11:22, Paul Sandoz wrote:
>> On May 13, 2014, at 11:34 AM, Seán Coffey  wrote:
>> 
>>> thanks for the comments. I hit a build issue when introducing some lambda 
>>> syntax to the corba repo : https://bugs.openjdk.java.net/browse/JDK-8042932
>>> 
>>> That's fixed now and I can continue with the corba push. I've cleaned up 
>>> the ORB class to make better use of generics and the diamond operator. 
>>> Removed some unused package imports also. Taken Daniel's suggestion to use 
>>> ConcurrentHashMap on board (and removed the sync block)
>>> 
>>> http://cr.openjdk.java.net/~coffeys/webrev.8042906.v2/webrev/
>>> 
>> There are also some compute-if-absent patterns that might be applicable:
>> 
>>  503 public LogWrapperBase getLogWrapper(String logDomain,
>>  504 String exceptionGroup, LogWrapperFactory factory)
>>  505 {
>>  506 StringPair key = new StringPair(logDomain, exceptionGroup);
>>  507
>>  508 LogWrapperBase logWrapper = wrapperMap.get(key);
>>  509 if (logWrapper == null) {
>>  510 logWrapper = factory.create(getLogger(logDomain));
>>  511 wrapperMap.put(key, logWrapper);
>>  512 }
>>  513
>>  514 return logWrapper;
>>  515 }
>>  516
>>  517 /** get the log wrapper class (its type is dependent on the 
>> exceptionGroup) for the
>>  518  * given log domain and exception group in this ORB instance.
>>  519  */
>>  520 public static LogWrapperBase staticGetLogWrapper(String logDomain,
>>  521 String exceptionGroup, LogWrapperFactory factory)
>>  522 {
>>  523 StringPair key = new StringPair(logDomain, exceptionGroup);
>>  524
>>  525 LogWrapperBase logWrapper = staticWrapperMap.get(key);
>>  526 if (logWrapper == null) {
>>  527 logWrapper = factory.create( staticGetLogger(logDomain));
>>  528 staticWrapperMap.put(key, logWrapper);
>>  529 }
>>  530
>>  531 return logWrapper;
>>  532 }
>> 
>>   return wrapperMap.computeIfAbsent(new StringPair(...),
>>   x -> factory.create());
>> 
>> Paul.
> 

Re: RFR: 8042906: Remove use of reflection in ORB

[Seán Coffey]

That's some nice code reduction Paul. Thanks.

http://cr.openjdk.java.net/~coffeys/webrev.8042906.v3/webrev/

I'll push these changes later unless I hear to the contrary.

regards,
Sean.

On 13/05/2014 11:22, Paul Sandoz wrote:

On May 13, 2014, at 11:34 AM, Seán Coffey  wrote:


thanks for the comments. I hit a build issue when introducing some lambda 
syntax to the corba repo : https://bugs.openjdk.java.net/browse/JDK-8042932

That's fixed now and I can continue with the corba push. I've cleaned up the 
ORB class to make better use of generics and the diamond operator. Removed some 
unused package imports also. Taken Daniel's suggestion to use ConcurrentHashMap 
on board (and removed the sync block)

http://cr.openjdk.java.net/~coffeys/webrev.8042906.v2/webrev/


There are also some compute-if-absent patterns that might be applicable:

  503 public LogWrapperBase getLogWrapper(String logDomain,
  504 String exceptionGroup, LogWrapperFactory factory)
  505 {
  506 StringPair key = new StringPair(logDomain, exceptionGroup);
  507
  508 LogWrapperBase logWrapper = wrapperMap.get(key);
  509 if (logWrapper == null) {
  510 logWrapper = factory.create(getLogger(logDomain));
  511 wrapperMap.put(key, logWrapper);
  512 }
  513
  514 return logWrapper;
  515 }
  516
  517 /** get the log wrapper class (its type is dependent on the 
exceptionGroup) for the
  518  * given log domain and exception group in this ORB instance.
  519  */
  520 public static LogWrapperBase staticGetLogWrapper(String logDomain,
  521 String exceptionGroup, LogWrapperFactory factory)
  522 {
  523 StringPair key = new StringPair(logDomain, exceptionGroup);
  524
  525 LogWrapperBase logWrapper = staticWrapperMap.get(key);
  526 if (logWrapper == null) {
  527 logWrapper = factory.create( staticGetLogger(logDomain));
  528 staticWrapperMap.put(key, logWrapper);
  529 }
  530
  531 return logWrapper;
  532 }

   return wrapperMap.computeIfAbsent(new StringPair(...),
   x -> factory.create());

Paul.

Re: RFR: 8042906: Remove use of reflection in ORB

[Paul Sandoz]

On May 13, 2014, at 11:34 AM, Seán Coffey  wrote:

> thanks for the comments. I hit a build issue when introducing some lambda 
> syntax to the corba repo : https://bugs.openjdk.java.net/browse/JDK-8042932
> 
> That's fixed now and I can continue with the corba push. I've cleaned up the 
> ORB class to make better use of generics and the diamond operator. Removed 
> some unused package imports also. Taken Daniel's suggestion to use 
> ConcurrentHashMap on board (and removed the sync block)
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8042906.v2/webrev/
> 

There are also some compute-if-absent patterns that might be applicable:

 503 public LogWrapperBase getLogWrapper(String logDomain,
 504 String exceptionGroup, LogWrapperFactory factory)
 505 {
 506 StringPair key = new StringPair(logDomain, exceptionGroup);
 507 
 508 LogWrapperBase logWrapper = wrapperMap.get(key);
 509 if (logWrapper == null) {
 510 logWrapper = factory.create(getLogger(logDomain));
 511 wrapperMap.put(key, logWrapper);
 512 }
 513 
 514 return logWrapper;
 515 }
 516 
 517 /** get the log wrapper class (its type is dependent on the 
exceptionGroup) for the
 518  * given log domain and exception group in this ORB instance.
 519  */
 520 public static LogWrapperBase staticGetLogWrapper(String logDomain,
 521 String exceptionGroup, LogWrapperFactory factory)
 522 {
 523 StringPair key = new StringPair(logDomain, exceptionGroup);
 524 
 525 LogWrapperBase logWrapper = staticWrapperMap.get(key);
 526 if (logWrapper == null) {
 527 logWrapper = factory.create( staticGetLogger(logDomain));
 528 staticWrapperMap.put(key, logWrapper);
 529 }
 530 
 531 return logWrapper;
 532 }

  return wrapperMap.computeIfAbsent(new StringPair(...), 
  x -> factory.create());

Paul.

Re: RFR: 8042906: Remove use of reflection in ORB

[Seán Coffey]

thanks for the comments. I hit a build issue when introducing some 
lambda syntax to the corba repo : 
https://bugs.openjdk.java.net/browse/JDK-8042932


That's fixed now and I can continue with the corba push. I've cleaned up 
the ORB class to make better use of generics and the diamond operator. 
Removed some unused package imports also. Taken Daniel's suggestion to 
use ConcurrentHashMap on board (and removed the sync block)


http://cr.openjdk.java.net/~coffeys/webrev.8042906.v2/webrev/

regards,
Sean.

On 12/05/2014 14:35, Mark Sheppard wrote:

OK thanks Chris and Daniel ... I see my misinterpretation now

regards
Mark

On 12/05/2014 13:46, Daniel Fuchs wrote:

Hi Mark,

AFAIKS computeIfAbsent does what we want:

 



It seems that putIfAbsent would not though. This is indeed confusing.
I wish putIfAbsent had been specified similarly to
computeIfAbsent...

best regards,

-- daniel

ConcurrentHashMap seems to have something more

On 5/12/14 1:33 PM, Mark Sheppard wrote:

If I read the javdoc correctly neither computeIfAbsent nor putIfAbsent
seem to match the
previous semantics of the if statement?

computeIfAbsent: "If the value for the specified key is present and
non-null, attempts to compute a new mapping given the key and its
current mapped value."
or
"If the specified key is not already associated with a value (or is
mapped to |null|), attempts to compute its value using the given 
mapping

function and enters it into this map unless |null|. "
my interpretation is this is relevant if value if there exist a value
and you want to update?

putIfAbsent: "If the specified key is not already associated with a
value (or is mapped to |null|) associates it with the given value and
returns |null|, else returns the current value."
in this case we can have null value returned

previous semantics were if the PM doesn't exist then instantiate and
cache it in the hashtable. return the PM - no null value returned

maybe I'm misinterpreting the docs

regards
Mark
On 12/05/2014 08:52, Seán Coffey wrote:

Good tip Alan/Chris.

I'll make that change and push.

Regards,
Sean.

On 12 May 2014 07:17:04 GMT+01:00, Alan Bateman
 wrote:

On 11/05/2014 21:56, Seán Coffey wrote:

Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can
remove use of reflection in ORB code to access the applet context.

bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/
I agree with Chris that computeIfAbsent is best here, that was 
also the


suggestion back in early April before the bootstrapping issue came 
up.

In any case, it's good you are re-visiting this to clean it up.

-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Mark Sheppard]

OK thanks Chris and Daniel ... I see my misinterpretation now

regards
Mark

On 12/05/2014 13:46, Daniel Fuchs wrote:

Hi Mark,

AFAIKS computeIfAbsent does what we want:

 



It seems that putIfAbsent would not though. This is indeed confusing.
I wish putIfAbsent had been specified similarly to
computeIfAbsent...

best regards,

-- daniel

ConcurrentHashMap seems to have something more

On 5/12/14 1:33 PM, Mark Sheppard wrote:

If I read the javdoc correctly neither computeIfAbsent nor putIfAbsent
seem to match the
previous semantics of the if statement?

computeIfAbsent: "If the value for the specified key is present and
non-null, attempts to compute a new mapping given the key and its
current mapped value."
or
"If the specified key is not already associated with a value (or is
mapped to |null|), attempts to compute its value using the given mapping
function and enters it into this map unless |null|. "
my interpretation is this is relevant if value if there exist a value
and you want to update?

putIfAbsent: "If the specified key is not already associated with a
value (or is mapped to |null|) associates it with the given value and
returns |null|, else returns the current value."
in this case we can have null value returned

previous semantics were if the PM doesn't exist then instantiate and
cache it in the hashtable. return the PM - no null value returned

maybe I'm misinterpreting the docs

regards
Mark
On 12/05/2014 08:52, Seán Coffey wrote:

Good tip Alan/Chris.

I'll make that change and push.

Regards,
Sean.

On 12 May 2014 07:17:04 GMT+01:00, Alan Bateman
 wrote:

On 11/05/2014 21:56, Seán Coffey wrote:

Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can
remove use of reflection in ORB code to access the applet context.

bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/
I agree with Chris that computeIfAbsent is best here, that was also 
the


suggestion back in early April before the bootstrapping issue came up.
In any case, it's good you are re-visiting this to clean it up.

-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Daniel Fuchs]

Hi Mark,

AFAIKS computeIfAbsent does what we want:



It seems that putIfAbsent would not though. This is indeed confusing.
I wish putIfAbsent had been specified similarly to
computeIfAbsent...

best regards,

-- daniel

ConcurrentHashMap seems to have something more

On 5/12/14 1:33 PM, Mark Sheppard wrote:

If I read the javdoc correctly neither computeIfAbsent nor putIfAbsent
seem to match the
previous semantics of the if statement?

computeIfAbsent: "If the value for the specified key is present and
non-null, attempts to compute a new mapping given the key and its
current mapped value."
or
"If the specified key is not already associated with a value (or is
mapped to |null|), attempts to compute its value using the given mapping
function and enters it into this map unless |null|. "
my interpretation is this is relevant if value if there exist a value
and you want to update?

putIfAbsent: "If the specified key is not already associated with a
value (or is mapped to |null|) associates it with the given value and
returns |null|, else returns the current value."
in this case we can have null value returned

previous semantics were if the PM doesn't exist then instantiate and
cache it in the hashtable. return the PM - no null value returned

maybe I'm misinterpreting the docs

regards
Mark
On 12/05/2014 08:52, Seán Coffey wrote:

Good tip Alan/Chris.

I'll make that change and push.

Regards,
Sean.

On 12 May 2014 07:17:04 GMT+01:00, Alan Bateman
 wrote:

On 11/05/2014 21:56, Seán Coffey wrote:

Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can
remove use of reflection in ORB code to access the applet context.

bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/

I agree with Chris that computeIfAbsent is best here, that was also the

suggestion back in early April before the bootstrapping issue came up.
In any case, it's good you are re-visiting this to clean it up.

-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Chris Hegarty]

On 12/05/14 12:33, Mark Sheppard wrote:

If I read the javdoc correctly neither computeIfAbsent nor putIfAbsent
seem to match the
previous semantics of the if statement?

computeIfAbsent: "If the value for the specified key is present and
non-null, attempts to compute a new mapping given the key and its
current mapped value."


I think this is from computeIfPresent, rather than computeIfAbsent.


or
"If the specified key is not already associated with a value (or is
mapped to |null|), attempts to compute its value using the given mapping
function and enters it into this map unless |null|. "
my interpretation is this is relevant if value if there exist a value
and you want to update?


Remapping is computeIfPresent.


putIfAbsent: "If the specified key is not already associated with a
value (or is mapped to |null|) associates it with the given value and
returns |null|, else returns the current value."
in this case we can have null value returned


Yes, computeIfAbsent is not a swap in replacement here.


previous semantics were if the PM doesn't exist then instantiate and
cache it in the hashtable. return the PM - no null value returned


The return value will be whatever, null of otherwise, is returned from 
setupPresentationManager, which is exactly what computeIfAbsent does.


-Chris.



maybe I'm misinterpreting the docs

regards
Mark
On 12/05/2014 08:52, Seán Coffey wrote:

Good tip Alan/Chris.

I'll make that change and push.

Regards,
Sean.

On 12 May 2014 07:17:04 GMT+01:00, Alan Bateman
 wrote:

On 11/05/2014 21:56, Seán Coffey wrote:

Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can
remove use of reflection in ORB code to access the applet context.

bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/

I agree with Chris that computeIfAbsent is best here, that was also the

suggestion back in early April before the bootstrapping issue came up.
In any case, it's good you are re-visiting this to clean it up.

-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Mark Sheppard]

If I read the javdoc correctly neither computeIfAbsent nor putIfAbsent 
seem to match the

previous semantics of the if statement?

computeIfAbsent: "If the value for the specified key is present and 
non-null, attempts to compute a new mapping given the key and its 
current mapped value."

or
"If the specified key is not already associated with a value (or is 
mapped to |null|), attempts to compute its value using the given mapping 
function and enters it into this map unless |null|. "
my interpretation is this is relevant if value if there exist a value 
and you want to update?


putIfAbsent: "If the specified key is not already associated with a 
value (or is mapped to |null|) associates it with the given value and 
returns |null|, else returns the current value."

in this case we can have null value returned

previous semantics were if the PM doesn't exist then instantiate and 
cache it in the hashtable. return the PM - no null value returned


maybe I'm misinterpreting the docs

regards
Mark
On 12/05/2014 08:52, Seán Coffey wrote:

Good tip Alan/Chris.

I'll make that change and push.

Regards,
Sean.

On 12 May 2014 07:17:04 GMT+01:00, Alan Bateman  wrote:

On 11/05/2014 21:56, Seán Coffey wrote:

Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can
remove use of reflection in ORB code to access the applet context.

bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/

I agree with Chris that computeIfAbsent is best here, that was also the

suggestion back in early April before the bootstrapping issue came up.
In any case, it's good you are re-visiting this to clean it up.

-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Seán Coffey]

Good tip Alan/Chris.

I'll make that change and push.

Regards,
Sean.

On 12 May 2014 07:17:04 GMT+01:00, Alan Bateman  wrote:
>On 11/05/2014 21:56, Seán Coffey wrote:
>> Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can 
>> remove use of reflection in ORB code to access the applet context.
>>
>> bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/
>I agree with Chris that computeIfAbsent is best here, that was also the
>
>suggestion back in early April before the bootstrapping issue came up. 
>In any case, it's good you are re-visiting this to clean it up.
>
>-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Daniel Fuchs]

I Seán,

I wonder whether it would be better to use a ConcurrentHashMap in
place of the plain HashMap and remove the synchronized lock
around putIfAbsent/computeIfAbsent?

best regards,

-- daniel

On 5/11/14 10:56 PM, Seán Coffey wrote:

Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can
remove use of reflection in ORB code to access the applet context.

bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/

regards,
Sean.

Re: RFR: 8042906: Remove use of reflection in ORB

[Alan Bateman]

On 11/05/2014 21:56, Seán Coffey wrote:
Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can 
remove use of reflection in ORB code to access the applet context.


bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/
I agree with Chris that computeIfAbsent is best here, that was also the 
suggestion back in early April before the bootstrapping issue came up. 
In any case, it's good you are re-visiting this to clean it up.


-Alan

Re: RFR: 8042906: Remove use of reflection in ORB

[Chris Hegarty]

Sean,

It is nice to see this code being cleaned up, and I agree with the changes.

You can use computeIfAbsent here, rather than, putIfAbsent, to avoid evaluating 
the result of setupPresentationManager() when there is already a mapping.
  pmContexts.computeIfAbsent(appletContext, 
PresentationManage::setupPresentationManager);

-Chris.

On 11 May 2014, at 21:56, Seán Coffey  wrote:

> Now that JDK 8 is the official build bootstrap JDK for JDK 9, we can remove 
> use of reflection in ORB code to access the applet context.
> 
> bug report : https://bugs.openjdk.java.net/browse/JDK-8042906
> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8042906/webrev/
> 
> regards,
> Sean.