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

[JENKINS-23271] - Process statuses of Remote process join() operations directly inside methods #2653

Merged
merged 2 commits into from Dec 3, 2016

Conversation

5 participants
@oleg-nenashev
Member

oleg-nenashev commented Nov 29, 2016

It is a follow-up to the response in JENKINS-23271. My original implementation in #2635 didn't take into the account the case when plugin developers implement their own Launchers and use them without ProcStarter.

The new implementation moves the explicit instance usage directly into the join() call

@reviewbybees @stephenc

if (LOGGER.isLoggable(Level.FINER)) {
LOGGER.log(Level.FINER, "Process {0} has finished with the return code {1}", new Object[]{procHolderForJoin, returnCode});
if (procHolderForJoin instanceof ProcWithJenkins23271Patch) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

Maybe it's an overkill since I just use the logic below for extra logging and a warning

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

Maybe it's an overkill since I just use the logic below for extra logging and a warning

@@ -1006,7 +1019,17 @@ public void kill() throws IOException, InterruptedException {
@Override
public int join() throws IOException, InterruptedException {
return process.join();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

Ideally kill() and isAlive() methods should be enhanced as well

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

Ideally kill() and isAlive() methods should be enhanced as well

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

I see no realistic case of isAlive() to be the last call against the Proc instance. So I just modified kill()

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

I see no realistic case of isAlive() to be the last call against the Proc instance. So I just modified kill()

@@ -431,7 +433,7 @@ private static String calcName(String[] cmd) {
* @deprecated as of 1.399. Replaced by {@link Launcher.RemoteLauncher.ProcImpl}
*/
@Deprecated
public static final class RemoteProc extends Proc {
public static final class RemoteProc extends Proc implements ProcWithJenkins23271Patch {
@stephenc

🐝 not that I'm happy about all these hacks... could we not just say "plugin bugs, fixie them plugins" instead

@@ -59,6 +59,7 @@
import java.util.logging.Logger;
import static org.apache.commons.io.output.NullOutputStream.NULL_OUTPUT_STREAM;
import hudson.Proc.ProcWithJenkins23271Patch;

This comment has been minimized.

@KostyaSha

KostyaSha Nov 29, 2016

Member

0_о what is this?

@KostyaSha

KostyaSha Nov 29, 2016

Member

0_о what is this?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

Just a restricted interface. It's better than to pollute the API

@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

Just a restricted interface. It's better than to pollute the API

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 29, 2016

Member

@stephenc

could we not just say "plugin bugs, fixie them plugins" instead
I would argue it's a plugin bug since they legitimately use the API. And the impacted plugin scope is potentially large. So I prefer such hack since it does not make the original one much worse...

Ideally I would like to have a way to add an annotation for the Proc class, something like @GCPleaseDoNotBeSoSmart, but I failed to find something relevant. Maybe @svanoort has seen something like that during his GC research

Member

oleg-nenashev commented Nov 29, 2016

@stephenc

could we not just say "plugin bugs, fixie them plugins" instead
I would argue it's a plugin bug since they legitimately use the API. And the impacted plugin scope is potentially large. So I prefer such hack since it does not make the original one much worse...

Ideally I would like to have a way to add an annotation for the Proc class, something like @GCPleaseDoNotBeSoSmart, but I failed to find something relevant. Maybe @svanoort has seen something like that during his GC research

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Nov 29, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@svanoort

This comment has been minimized.

Show comment
Hide comment
@svanoort

svanoort Nov 30, 2016

Member

@oleg-nenashev You can disable escape analysis with a JVM flag, but you really shouldn't if performance matters (stack allocation is faster than heap allocation and garbage-free). From the information I've seen, escape analysis isn't done until a method is inlined. There are ways to prevent inlining -- making the method too long, the invocation patterns too complex, or not invoking it often enough. But you shouldn't do that, because inlining and aggressive compilation are what allow JIT to be faster.

One useful aspect of this for testing: IIRC there are JVM flags to force earlier JIT compilation & inlining more readily (for example after only 1 execution). There also exist ways to force inlining on specific methods (which might be useful for testing in other cases) -- http://nicoulaj.github.io/compile-command-annotations/ -- but they're brittle, more useful for testing than runtime use.

The proper workaround is basically what you're doing now -- do something with the object so it isn't killed prematurely due to escape analysis. It's essentially a bug to rely on a java object surviving without strong references (barring use of Soft/Weak/Phantom references).

BTW a version of this is a common problem in Java microbenchmarks: if the JVM detects you don't do anything with the output of a function call, and certain invariants can be proven, the call will be optimized away. The solution is generally to do something with the result (the JMH framework adds a BlackHole utility class that can insure the method is still invoked).

Member

svanoort commented Nov 30, 2016

@oleg-nenashev You can disable escape analysis with a JVM flag, but you really shouldn't if performance matters (stack allocation is faster than heap allocation and garbage-free). From the information I've seen, escape analysis isn't done until a method is inlined. There are ways to prevent inlining -- making the method too long, the invocation patterns too complex, or not invoking it often enough. But you shouldn't do that, because inlining and aggressive compilation are what allow JIT to be faster.

One useful aspect of this for testing: IIRC there are JVM flags to force earlier JIT compilation & inlining more readily (for example after only 1 execution). There also exist ways to force inlining on specific methods (which might be useful for testing in other cases) -- http://nicoulaj.github.io/compile-command-annotations/ -- but they're brittle, more useful for testing than runtime use.

The proper workaround is basically what you're doing now -- do something with the object so it isn't killed prematurely due to escape analysis. It's essentially a bug to rely on a java object surviving without strong references (barring use of Soft/Weak/Phantom references).

BTW a version of this is a common problem in Java microbenchmarks: if the JVM detects you don't do anything with the output of a function call, and certain invariants can be proven, the call will be optimized away. The solution is generally to do something with the result (the JMH framework adds a BlackHole utility class that can insure the method is still invoked).

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Nov 30, 2016

Member

@svanoort Thanks! I've never expected GC escape analysis to kill objects within their method calls, but I am pretty convinced we see it here. It may be a major pain point for remoting, because it's possible to imagine such call patterns outside Jenkins remote process management. Potentially we could make remoteable resources closeable and thus enforce proper GC-proof usage patterns, but it would require some efforts.

Member

oleg-nenashev commented Nov 30, 2016

@svanoort Thanks! I've never expected GC escape analysis to kill objects within their method calls, but I am pretty convinced we see it here. It may be a major pain point for remoting, because it's possible to imagine such call patterns outside Jenkins remote process management. Potentially we could make remoteable resources closeable and thus enforce proper GC-proof usage patterns, but it would require some efforts.

@svanoort

This comment has been minimized.

Show comment
Hide comment
@svanoort

svanoort Nov 30, 2016

Member

@oleg-nenashev If you can reproduce the issue consistently, try using the JVM argument -XX:-DoEscapeAnalysis -- if the issue disappears it is escape analysis. If not, something else.

Probably a good idea to confirm before we do any deep-down restructuring -- if the culprit isn't what we think the fix might be much simpler.

Member

svanoort commented Nov 30, 2016

@oleg-nenashev If you can reproduce the issue consistently, try using the JVM argument -XX:-DoEscapeAnalysis -- if the issue disappears it is escape analysis. If not, something else.

Probably a good idea to confirm before we do any deep-down restructuring -- if the culprit isn't what we think the fix might be much simpler.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment

@oleg-nenashev oleg-nenashev merged commit 2989335 into jenkinsci:master Dec 3, 2016

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@oleg-nenashev oleg-nenashev deleted the oleg-nenashev:bug/JENKINS-23271-take3 branch Dec 3, 2016

daniel-beck added a commit that referenced this pull request Dec 5, 2016

olivergondza added a commit that referenced this pull request Dec 8, 2016

[JENKINS-23271] - Process statuses of Remote process join() operation…
…s directly inside methods (#2653)

* [JENKINS-23271] - Process statuses of Remote process join() operations directly inside methods

* [JENKINS-23271] - Also prevent the issue when the kill() command is the last call in the usage sequence

(cherry picked from commit 2989335)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment