-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8288140: Avoid redundant Hashtable.get call in Signal.handle #9100
8288140: Avoid redundant Hashtable.get call in Signal.handle #9100
Conversation
👋 Welcome back aturbanov! A progress list of the required criteria for merging this PR into |
update copyright year
@turbanoff The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
Signal.Handler oldHandler = handlers.remove(sig); | ||
if (newH == 2) { | ||
handlers.put(sig, handler); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you made this change instead:
Signal.Handler oldHandler = handlers.remove(sig); | |
if (newH == 2) { | |
handlers.put(sig, handler); | |
} | |
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, yes you are correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andhandlers
maps respectively, assuming their inter-referenced state is consistent. Buthandle
method may be concurrently modifying them, sodispatch
may see updated state ofsignals
map while seeing old state ofhandlers
map or vice versa. - race no. 2: besides
signals
andhandlers
there is a 3rd map in native code, updated withhandle0
native method. Native code dispatches signals according to that native map, forwarding them to either native handlers or todispatch
Java method. Buthandle
method may be modifying that native map concurrently, so dispatch may be called as a consequence of updated native map while seeing old states ofsignals
andhandlers
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tohandle0
native method. You could even update thesignals
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 thesignals
map in that case to clean-up.
The possible case when the lookup intohandlers
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 anddispatch
is therefore already called, buthandlers
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to reproduce this possible problem via jcstress, but never caught this race.
package org.openjdk.jcstress.samples.jmm.basic;
import org.openjdk.jcstress.annotations.*;
import org.openjdk.jcstress.infra.results.ZZZZZZZZ_Result;
import sun.misc.Signal;
import sun.misc.SignalHandler;
import java.io.IOException;
import static org.openjdk.jcstress.annotations.Expect.*;
@JCStressTest
@Outcome(id = "false, false, false, false, false, false, false, false", expect = ACCEPTABLE, desc = "All results are ok")
@Outcome(id = ".*", expect = ACCEPTABLE_INTERESTING, desc = "All results are ok")
@State
public class BasicJMM_11_Signal {
/*
How to run this test:
$ /home/turbanoff/.jdks/liberica-18.0.1.1/bin/java -jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
$ /home/turbanoff/Projects/official_jdk/build/linux-x86_64-server-fastdebug/jdk/bin/java -jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
$ /home/turbanoff/Projects/jdk2/build/linux-x86_64-server-release/jdk/bin/java -jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
*/
private static final Signal[] signals = new Signal[]{
new Signal("URG"),
// new Signal("USR2"),
// new Signal("ALRM"),
// new Signal("USR1"),
new Signal("CHLD"),
new Signal("XFSZ"),
new Signal("CONT"),
// new Signal("POLL"),
new Signal("WINCH"),
// new Signal("IO"),
new Signal("PIPE"),
// new Signal("HUP"),
// new Signal("POLL"),
// new Signal("PROF"),
// new Signal("INT"),
// new Signal("STKFLT"),
new Signal("TSTP"),
// new Signal("SYS"),
// new Signal("TERM"),
// new Signal("TRAP"),
// new Signal("ABRT"),
new Signal("TTOU"),
new Signal("TTIN"),
// new Signal("BUS"),
// new Signal("VTALRM"),
// new Signal("XCPU"),
// new Signal("PWR")
};
private static final String[][] commands = new String[signals.length][];
private static final long pid = ProcessHandle.current().pid();
static {
for (int i = 0; i < commands.length; i++) {
commands[i] = new String[]{ "kill", "-" + signals[i].getName(), Long.toString(pid) };
}
}
@Actor
public void thread1() {
for (String[] command : commands) {
try {
new ProcessBuilder(command)
.directory(null)
.start();
} catch (IOException e) {
e.printStackTrace();
}
}
}
@Actor
public void thread2(ZZZZZZZZ_Result r) {
for (int i = 0; i < signals.length; i++) {
Signal signal = signals[i];
Signal.handle(signal, new MySignalHandler(i, r));
}
}
private static class MySignalHandler implements SignalHandler {
private final int num;
private final ZZZZZZZZ_Result r;
public MySignalHandler(int num, ZZZZZZZZ_Result r) {
this.num = num;
this.r = r;
}
@Override
public void handle(Signal sig) {
switch (num) {
case 0:
r.r1 = true;
break;
case 1:
r.r2 = true;
break;
case 2:
r.r3 = true;
break;
case 3:
r.r4 = true;
case 4:
r.r5 = true;
break;
case 5:
r.r6 = true;
break;
case 6:
r.r7 = true;
break;
case 7:
r.r8 = true;
break;
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe fixing this race is out of scope of current PR. Feel free to open a separate issue :)
apply dmlloyd's suggestion
oldHandler = handlers.put(sig, handler); | ||
} else { | ||
oldHandler = handlers.remove(sig); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ternary assignment might be an alternative here:
Signal.Handler oldHandler = (newH == 2) ? handlers.put(sig, handler) : handlers.remove(sig);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much logic in the single line. I like if
approach more.
@turbanoff This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 25 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Thank you for review! |
Going to push as commit dfeeb6f.
Your commit was automatically rebased without conflicts. |
@turbanoff Pushed as commit dfeeb6f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
jdk/src/java.base/share/classes/jdk/internal/misc/Signal.java
Lines 177 to 178 in bc28bae
Instead of separate Hashtable.get/remove calls we can just use value returned by
remove
,It results in cleaner and a bit faster code.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9100/head:pull/9100
$ git checkout pull/9100
Update a local copy of the PR:
$ git checkout pull/9100
$ git pull https://git.openjdk.org/jdk pull/9100/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9100
View PR using the GUI difftool:
$ git pr show -t 9100
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9100.diff