Addition to MethodHandle: invokeExactUnchecked(()
At present, MethodHandle's invokeExact() is declared as the following:
>From [1]:
> @IntrinsicCandidate
> public final native @PolymorphicSignature Object invokeExact(Object...
> args) throws Throwable;
I think this is a case where this method should probably never have
thrown a checked exception and I'd like to make the case and
test the waters and see if others have the appetite to see this change. I'm
assuming that altering this function signature is a no-go and a new
invokeExactUnchecked() is preferable.
Making the Case
I know, I know there's no PL bikeshed greater than that checked vs
unchecked exceptions -- looking at you, Future.get() -- but I think this
case is fundamentally different. MethodHandles are kindof a low-level niche
unit of JVM magic. That's why it's already an *exceptional* function given
its @PolymorphicSignature. Pun intended.
Most APIs for general use *should* drive good, sane programming habits, but
if you're reaching for MethodHandles you're probably doing something you
want a good deal of control over. There are a ton of ways it's already
expected to be a "just trust me" escape hatch already. MethodHandles are
often used with the expectation that they have to be static finals for them
to be inlined as if they are regular code. It is very awkward
and counter-productive even to have to introduce a *try { } catch(Throwable
e) { } *with all of its syntactic, bytecode, *and* JIT/inlining baggage.
Just throwing the invokeExact() call in a function or a lambda to mask out
the try-catch just pours back the layers you're futzing around with
MethoHandles to peel back.
If there's an interest and an appetite for it I'd love to take a crack at
implementing it. Any pointers are welcome!
[0]: https://bugs.openjdk.java.net/browse/JDK-8254354
[1]:
https://github.com/openjdk/jdk/blame/master/src/java.base/share/classes/java/lang/invoke/MethodHandle.java
Re: Very fast List/Deque to java.util?
I'd like to point out that rodde has also asked this project to be included in Apache Commons Collections: https://lists.apache.org/thread/klwtkbz96rp9zfr1077sc9r8tjtthfl4 On 11/06/2022 15:53, - wrote: Hello, What do you think? First, for your benchmarks, I recommend you write them with https://github.com/openjdk/jmh, a library that allows configuration of warmup time, number of trials, and has utilities that avoid jvm optimization's impact on your benchmarks. A few cents: I recall seeing a similar proposal before. This implementation is definitely superior to the Linked List as an implementation of both deque and list at the cost of an extra finger array allocation, but it probably won't excel compared to dedicated Deque or List implementations in terms of performance and memory usage. Modern JDK List/Deque users usually use the array-focused implementations like ArrayDeque and ArrayList; even though ArrayList is slow at adding or removing at non-tail, most usages of ArrayList is iterating and sometimes adding (predominantly at tail). Array iteration is better optimized by hardware due to arrays. Thus, LinkedList is already out-of-favor like Vector, and there are relevant discussions in https://github.com/openjdk/jdk/pull/2744. The doubly-linked structure of the finger list is at a natural disadvantage (for iteration) here. Even though this finger list won't be a replacement for ArrayList or ArrayDeque, I wonder if it can be modified to become a concurrent collection, so that modifications to the list can be synchronized by the intervals between fingers. If it can, it would probably provide an efficient concurrent list implementation compared to the current CopyOnWriteArrayList, even though your current implementation seems not to support concurrency. Does it deserve to be included in JDK? Let's revisit your proposal again: if it's in the JDK, where will it be used? After all, JDK is a library that is used by millions or billions of programs. Feel free to come up with a few use cases and we will see if this finger list is the best for those cases. On Sat, Jun 11, 2022 at 2:39 AM Rodion Efremov wrote: Hi, I have this List/Deque implementation that runs (in a rather versatile benchmark) much faster than ArrayList and LinkedList: https://github.com/coderodde/IndexedLinkedList What do you think? Does it deserve to be included in JDK? Best regards, rodde
Re: Very fast List/Deque to java.util?
Hello, > What do you think? First, for your benchmarks, I recommend you write them with https://github.com/openjdk/jmh, a library that allows configuration of warmup time, number of trials, and has utilities that avoid jvm optimization's impact on your benchmarks. A few cents: I recall seeing a similar proposal before. This implementation is definitely superior to the Linked List as an implementation of both deque and list at the cost of an extra finger array allocation, but it probably won't excel compared to dedicated Deque or List implementations in terms of performance and memory usage. Modern JDK List/Deque users usually use the array-focused implementations like ArrayDeque and ArrayList; even though ArrayList is slow at adding or removing at non-tail, most usages of ArrayList is iterating and sometimes adding (predominantly at tail). Array iteration is better optimized by hardware due to arrays. Thus, LinkedList is already out-of-favor like Vector, and there are relevant discussions in https://github.com/openjdk/jdk/pull/2744. The doubly-linked structure of the finger list is at a natural disadvantage (for iteration) here. Even though this finger list won't be a replacement for ArrayList or ArrayDeque, I wonder if it can be modified to become a concurrent collection, so that modifications to the list can be synchronized by the intervals between fingers. If it can, it would probably provide an efficient concurrent list implementation compared to the current CopyOnWriteArrayList, even though your current implementation seems not to support concurrency. > Does it deserve to be included in JDK? Let's revisit your proposal again: if it's in the JDK, where will it be used? After all, JDK is a library that is used by millions or billions of programs. Feel free to come up with a few use cases and we will see if this finger list is the best for those cases. On Sat, Jun 11, 2022 at 2:39 AM Rodion Efremov wrote: > > Hi, > > I have this List/Deque implementation that runs (in a rather versatile > benchmark) much faster than ArrayList and LinkedList: > > https://github.com/coderodde/IndexedLinkedList > > What do you think? Does it deserve to be included in JDK? > > Best regards, > rodde
Re: RFR: 8287696: Avoid redundant Hashtable.containsKey call in JarVerifier.doneWithMeta
On Sat, 28 May 2022 12:00:00 GMT, Andrey Turbanov wrote: > Hashtable doesn't allow `null` values. So, instead of pair > `containsKey`/`remove` calls, we can directly call `remove` and then compare > result with `null`. > https://github.com/openjdk/jdk/blob/2c461acfebd28fe5ef62805cbb004f91a3b18f08/src/java.base/share/classes/java/util/jar/JarVerifier.java#L433-L436 Thank you for review! - PR: https://git.openjdk.org/jdk/pull/8935
Integrated: 8287696: Avoid redundant Hashtable.containsKey call in JarVerifier.doneWithMeta
On Sat, 28 May 2022 12:00:00 GMT, Andrey Turbanov wrote: > Hashtable doesn't allow `null` values. So, instead of pair > `containsKey`/`remove` calls, we can directly call `remove` and then compare > result with `null`. > https://github.com/openjdk/jdk/blob/2c461acfebd28fe5ef62805cbb004f91a3b18f08/src/java.base/share/classes/java/util/jar/JarVerifier.java#L433-L436 This pull request has now been integrated. Changeset: f1143b1b Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/f1143b1b57683665c81d24ff192a9babc30f28ea Stats: 7 lines in 1 file changed: 0 ins; 5 del; 2 mod 8287696: Avoid redundant Hashtable.containsKey call in JarVerifier.doneWithMeta Reviewed-by: jpai, lancea - PR: https://git.openjdk.org/jdk/pull/8935
Re: RFR: 8288140: Avoid redundant Hashtable.get call in Signal.handle [v2]
On Sat, 11 Jun 2022 07:55:52 GMT, Peter Levart wrote: >> Oops, yes you are correct! > > Hi, > I think the synchronized block was redundant already in original code. Since > the entire handle method is `static synchronized` and it is the only method > that modifies the `handlers` and `signals` maps. > But even with so much redundant synchronization, the Signal class is not > without races. There are multiple possible races in `dispatch(int number)` > method which is called from native code to dispatch a signal: > - race no. 1: dispatch method (not synchronized) performs 2 independent > lookups into `signals` and `handlers` maps respectively, assuming their > inter-referenced state is consistent. But `handle` method may be concurrently > modifying them, so `dispatch` may see updated state of `signals` map while > seeing old state of `handlers` map or vice versa. > - race no. 2: besides `signals` and `handlers` there is a 3rd map in native > code, updated with `handle0` native method. Native code dispatches signals > according to that native map, forwarding them to either native handlers or to > `dispatch` Java method. But `handle` method may be modifying that native map > concurrently, so dispatch may be called as a consequence of updated native > map while seeing old states of `signals` and `handlers` maps. > > I'm sure I might have missed some additional races. > > How to fix this? Is it even necessary? I think that this internal API is not > used frequently so this was hardly an issue. But anyway, it would be a > challenge. While the two java maps: `handlers` and `signals` could be > replaced with a single map, there is the 3rd map in native code. We would not > want to make `dispatch` method synchronized, so with careful ordering of > modifications it is perhaps possible to account for races and make them > harmless... > WDYT? Well, studying this further, I think some races are already accounted for and are harmless except one. The `handle` method 1st attempts to register handler in native code via call to `handle0` and then updates the `signals` map.. As soon as native code registers the handler, `dispatch` may be called and the lookup into `signals` map may return null which would cause NPE in the following lookup into `handlers` map. So there are two possibilities to fix this: - guard for null result from `singnals` lookup, or - swap the order of modifying the `signals` map and a call to `handle0` native method. You could even update the `signals` map with `.putIfAbsent()` to avoid multiple reassignment with otherwise equal instances. This may unnecessarily establish a mapping for a Signal the can later not be registered, so you can remove it from the `signals` map in that case to clean-up. The possible case when the lookup into `handlers` map returns null Handler is already taken into account, but there is a possible case where a NativeHandler is replaced with a java Handler implementation and `dispatch` is therefore already called, but `handlers` map lookup still returns a NativeHandler. In that case the NativeHandler::handle method will throw exception. To avoid that, NativeHandler::handle could be an empty method instead. - PR: https://git.openjdk.org/jdk/pull/9100
Re: RFR: 8288140: Avoid redundant Hashtable.get call in Signal.handle [v2]
On Fri, 10 Jun 2022 11:45:28 GMT, David M. Lloyd wrote: >> Hello David, I suspect you mean `handlers.put(sig, handler)` and not >> `handlers.replace(...)`? And yes, I think what you suggest will help remove >> the synchronized block here. > > Oops, yes you are correct! Hi, I think the synchronized block was redundant already in original code. Since the entire handle method is `static synchronized` and it is the only method that modifies the `handlers` and `signals` maps. But even with so much redundant synchronization, the Signal class is not without races. There are multiple possible races in `dispatch(int number)` method which is called from native code to dispatch a signal: - race no. 1: dispatch method (not synchronized) performs 2 independent lookups into `signals` and `handlers` maps respectively, assuming their inter-referenced state is consistent. But `handle` method may be concurrently modifying them, so `dispatch` may see updated state of `signals` map while seeing old state of `handlers` map or vice versa. - race no. 2: besides `signals` and `handlers` there is a 3rd map in native code, updated with `handle0` native method. Native code dispatches signals according to that native map, forwarding them to either native handlers or to `dispatch` Java method. But `handle` method may be modifying that native map concurrently, so dispatch may be called as a consequence of updated native map while seeing old states of `signals` and `handlers` maps. I'm sure I might have missed some additional races. How to fix this? Is it even necessary? I think that this internal API is not used frequently so this was hardly an issue. But anyway, it would be a challenge. While the two java maps: `handlers` and `signals` could be replaced with a single map, there is the 3rd map in native code. We would not want to make `dispatch` method synchronized, so with careful ordering of modifications it is perhaps possible to account for races and make them harmless... WDYT? - PR: https://git.openjdk.org/jdk/pull/9100
