RFR: 8042906: Remove use of reflection in ORB
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.
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 <sean.coffey@oracle.com> 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.
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
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 <Alan.Bateman@oracle.com> 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
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 <Alan.Bateman@oracle.com> 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
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 <Alan.Bateman@oracle.com> 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
Hi Mark, AFAIKS computeIfAbsent does what we want: <http://docs.oracle.com/javase/8/docs/api/java/util/Map.html#computeIfAbsent-K-java.util.function.Function-> 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 <Alan.Bateman@oracle.com> 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
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 <Alan.Bateman@oracle.com> 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
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 <Alan.Bateman@oracle.com> 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
On May 13, 2014, at 11:34 AM, Seán Coffey <sean.coffey@oracle.com> 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.
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 <sean.coffey@oracle.com> 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.
On 13 May 2014, at 19:53, Seán Coffey <sean.coffey@oracle.com> 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 <sean.coffey@oracle.com> 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.
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.
participants (6)
-
Alan Bateman
-
Chris Hegarty
-
Daniel Fuchs
-
Mark Sheppard
-
Paul Sandoz
-
Seán Coffey