Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Thread not killable when in 300_000_000.times { |i| var = i } loop #5199

Open
pitr-ch opened this issue May 27, 2018 · 9 comments
Open

Thread not killable when in 300_000_000.times { |i| var = i } loop #5199

pitr-ch opened this issue May 27, 2018 · 9 comments

Comments

@pitr-ch
Copy link
Member

pitr-ch commented May 27, 2018 •
edited
Loading

Environment

-> jruby -v
jruby 9.1.17.0 (2.3.3) 2018-04-20 d8b1ff9 Java HotSpot(TM) 64-Bit Server VM 25.71-b01-internal-jvmci-0.42 on 1.8.0_161-b12 +jit [darwin-x86_64]

in a snippet

var     = 0
t = Thread.new do
  300_000_000.times { |i| var = i }
end

Thread.pass while var < 1

p t.status
p Time.now
p t.kill
p t.join
p Time.now

Expected Behavior

the thread is killed immediately

Actual Behavior

however It runs until the loop finishes
@headius
Copy link
Member

headius commented May 28, 2018

Not enough checkpointing for thread events in that case, perhaps because there's no calls and no backward branch. I wouldn't consider this representative of any real code, but we should measure the impact of adding a checkpoint to, say, the block exiting.

@subbuss @enebo Thoughts on this one?

@pitr-ch
Copy link
Member Author

pitr-ch commented May 28, 2018 •
edited
Loading

Personally I think it should not be possible to write a code in Ruby which would be un-killable. I originally found out by not being able to terminate https://github.com/ruby-concurrency/concurrent-ruby/blob/master/spec/concurrent/synchronization_spec.rb#L15-L20 which less minimal.

@subbuss
Copy link
Contributor

subbuss commented May 28, 2018

@headius, There is a back-edge ... except it is in the jruby library code. That indicates that the library code is missing the checkpoint maybe?

@enebo
Copy link
Member

enebo commented May 28, 2018

Shouldn't the times have a threadpoll instr right before jump? (Not convenient for me to check right now).

@headius
Copy link
Member

headius commented May 30, 2018

@enebo It would if it were implemented in Ruby. In Java we have to manually add polling to backedges.

@headius
Copy link
Member

headius commented May 30, 2018

@enebo I guess I'm saying yes, it probably should :-)

@headius
Copy link
Member

headius commented May 30, 2018

@pitr-ch I think we agree. I just wanted to point out this is a rather unlikely scenario. Nearly all code will have at least one call, which will trigger the polling.

@enebo
Copy link
Member

enebo commented May 30, 2018

ah yeah times is native. No call here so times is missing the magic itself. Not an IR bug at least.

@headius
Copy link
Member

headius commented Jul 18, 2020

The following patch fixes this with a minimum of overhead (only checking thread events every N calls):

diff --git a/core/src/main/java/org/jruby/RubyFixnum.java b/core/src/main/java/org/jruby/RubyFixnum.java
index a97878ce99..6cdcce4c56 100644
--- a/core/src/main/java/org/jruby/RubyFixnum.java
+++ b/core/src/main/java/org/jruby/RubyFixnum.java
@@ -278,17 +278,26 @@ public class RubyFixnum extends RubyInteger implements Constantizable {
                     final IRubyObject nil = context.nil;
                     for (long i = 0; i < value; i++) {
                         block.yieldSpecific(context, nil);
+                        if ((i & ThreadContext.CALL_POLL_COUNT) == 0) {
+                            context.pollThreadEvents();
+                        }
                     }
                 } else {
                     // no arg needed
                     for (long i = 0; i < value; i++) {
                         block.yieldSpecific(context);
+                        if ((i & ThreadContext.CALL_POLL_COUNT) == 0) {
+                            context.pollThreadEvents();
+                        }
                     }
                 }
             } else {
                 final Ruby runtime = context.runtime;
                 for (long i = 0; i < value; i++) {
                     block.yield(context, RubyFixnum.newFixnum(runtime, i));
+                    if ((i & ThreadContext.CALL_POLL_COUNT) == 0) {
+                        context.pollThreadEvents();
+                    }
                 }
             }
             return this;
diff --git a/core/src/main/java/org/jruby/runtime/ThreadContext.java b/core/src/main/java/org/jruby/runtime/ThreadContext.java
index 0318639a2c..7992a3e9e3 100644
--- a/core/src/main/java/org/jruby/runtime/ThreadContext.java
+++ b/core/src/main/java/org/jruby/runtime/ThreadContext.java
@@ -97,7 +97,7 @@ public final class ThreadContext {
     private final static int INITIAL_FRAMES_SIZE = 10;
 
     /** The number of calls after which to do a thread event poll */
-    private final static int CALL_POLL_COUNT = 0xFFF;
+    public final static int CALL_POLL_COUNT = 0xFFF;
 
     // runtime, nil, and runtimeCache cached here for speed of access from any thread
     public final Ruby runtime;

However it does reduce performance to use even the "call" version of polling:

BEFORE:
$ jruby -e "loop { t = Time.now; 100_000_000.times { }; puts Time.now - t }"
0.542925
0.567139
0.627118
0.507028
0.508385
0.512144
0.507685

AFTER:
$ jruby -e "loop { t = Time.now; 100_000_000.times { }; puts Time.now - t }"
0.557857
0.600398
0.593976
0.5913959999999999
0.601807
0.5930259999999999

Note that the performance of a pure-Ruby version, when the stars align (this is on GraalVM), both works and performs well (since the thread interrupt is handled as a safepoint):

BEFORE:
$ jruby -Xcompile.invokedynamic -Xfixnum.cache=false -e "class Integer; def times; i = 0; while i < self; yield i; i += 1; end; end; end; loop { t = Time.now; 100_000_000.times { }; puts Time.now - t }"
0.213364
0.17784799999999998
0.11926099999999999
0.12261899999999999
0.11895299999999999
0.11813799999999999

AFTER:
$ jruby -Xcompile.invokedynamic -Xfixnum.cache=false -e "class Integer; def times; i = 0; while i < self; yield i; i += 1; end; end; end; loop { t = Time.now; 100_000_000.times { }; puts Time.now - t }"
0.158944
0.176442
0.119419
0.122568
0.119013
0.121836

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@headius @enebo @subbuss @pitr-ch