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
Re: RFR: 8288140: Avoid redundant Hashtable.get call in Signal.handle [v2]
On Fri, 10 Jun 2022 11:31:06 GMT, Andrey Turbanov wrote:
>> https://github.com/openjdk/jdk/blob/bc28baeba9360991e9b7575e1fbe178d873ccfc1/src/java.base/share/classes/jdk/internal/misc/Signal.java#L177-L178
>>
>> Instead of separate Hashtable.get/remove calls we can just use value
>> returned by `remove`,
>> It results in cleaner and a bit faster code.
>
> Andrey Turbanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8288140: Avoid redundant Hashtable.get call in Signal.handle
> apply dmlloyd's suggestion
Marked as reviewed by rriggs (Reviewer).
src/java.base/share/classes/jdk/internal/misc/Signal.java line 181:
> 179: } else {
> 180: oldHandler = handlers.remove(sig);
> 181: }
A ternary assignment might be an alternative here:
Signal.Handler oldHandler = (newH == 2) ? handlers.put(sig, handler) :
handlers.remove(sig);
-
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 06:45:00 GMT, Jaikiran Pai wrote:
>> src/java.base/share/classes/jdk/internal/misc/Signal.java line 180:
>>
>>> 178: if (newH == 2) {
>>> 179: handlers.put(sig, handler);
>>> 180: }
>>
>> If you made this change instead:
>>
>> Suggestion:
>>
>> Signal.Handler oldHandler;
>> if (newH == 2) {
>> oldHandler = handlers.put(sig, handler);
>> } else {
>> oldHandler = handlers.remove(sig);
>> }
>>
>>
>> Wouldn't you be able to remove the entire `synchronized` block?
>
> 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!
-
PR: https://git.openjdk.org/jdk/pull/9100
Re: RFR: 8288140: Avoid redundant Hashtable.get call in Signal.handle [v2]
> https://github.com/openjdk/jdk/blob/bc28baeba9360991e9b7575e1fbe178d873ccfc1/src/java.base/share/classes/jdk/internal/misc/Signal.java#L177-L178 > > Instead of separate Hashtable.get/remove calls we can just use value returned > by `remove`, > It results in cleaner and a bit faster code. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8288140: Avoid redundant Hashtable.get call in Signal.handle apply dmlloyd's suggestion - Changes: - all: https://git.openjdk.org/jdk/pull/9100/files - new: https://git.openjdk.org/jdk/pull/9100/files/220d3b61..829f9e23 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9100=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9100=00-01 Stats: 17 lines in 1 file changed: 0 ins; 0 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/9100.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9100/head:pull/9100 PR: https://git.openjdk.org/jdk/pull/9100
