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.

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

Add thread polling on backedges of Range#each loop #7283

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

headius
Copy link
Member

@headius headius commented Aug 1, 2022

There are several forms of loops here but none had any explicit
thread polling, which led to #7279 and timeout events not firing
during trivial Range#each loops. This commit adds polling to all
loops reachable from Range#each, which fixees #7279 and other
untested cases.

This does, however, come with a performance hit; the related thread
state must be checked after each loop. These could be improved in
the future by using a safe point mechanism like SwitchPoint, or by
porting these functions to Ruby which gets backedge thread polling
via the IR.

Fixes #7279

There are several forms of loops here but none had any explicit
thread polling, which led to jruby#7279 and timeout events not firing
during trivial Range#each loops. This commit adds polling to all
loops reachable from Range#each, which fixees jruby#7279 and other
untested cases.

This does, however, come with a performance hit; the related thread
state must be checked after each loop. These could be improved in
the future by using a safe point mechanism like SwitchPoint, or by
porting these functions to Ruby which gets backedge thread polling
via the IR.

Fixes jruby#7279
@headius headius added this to the JRuby 9.4.0.0 milestone Aug 1, 2022
@k77ch7
Copy link
Contributor

k77ch7 commented Aug 5, 2022

Certainly, this change affects performance.

@enebo
Copy link
Member

enebo commented Aug 5, 2022

@headius @k77ch7 We could try and measure impact but we can also opt for callThreadPoll() which will only poll every 0xFFF calls. We could also customize modulos how many backedges to pass to check? I think it is important to see if this impact is something we would really notice or not though. Looks like this will do 2 native calls and one field read.

@k77ch7
Copy link
Contributor

k77ch7 commented Aug 5, 2022

I try and measure impact. There may be no practical impact.
I think we can merge.

Test script

n = 10000000
Benchmark.bm do |x|
  5.times do 
    x.report do 
      100.times do
        for i in 0..n do; (i + i % (i+1)) % (i + 10) ; end
      end
    end
  end
end

Results

jruby-head(9.4.0.0-SNAPSHOT)

       user     system      total        real
  36.290000   0.290000  36.580000 ( 35.334764)
  35.790000   0.170000  35.960000 ( 35.057817)
  35.740000   0.170000  35.910000 ( 35.002649)
  35.940000   0.170000  36.110000 ( 35.277768)
  35.800000   0.190000  35.990000 ( 35.108502)

jruby-head(9.4.0.0-SNAPSHOT) & this patch

       user     system      total        real
  37.560000   0.340000  37.900000 ( 37.015159)
  35.990000   0.180000  36.170000 ( 35.259777)
  36.090000   0.180000  36.270000 ( 35.290459)
  36.020000   0.170000  36.190000 ( 35.259498)
  36.050000   0.170000  36.220000 ( 35.269742)

mri 3.1.2

       user     system      total        real
  55.181839   0.048456  55.230295 ( 55.238001)
  55.186167   0.043971  55.230138 ( 55.234660)
  55.218860   0.046993  55.265853 ( 55.280426)
  55.279925   0.059030  55.338955 ( 55.374247)
  55.158043   0.048078  55.206121 ( 55.218978)

My env

  • JRuby version
jruby 9.4.0.0-SNAPSHOT (3.1.0) 2022-08-05 808fbf11f2 OpenJDK 64-Bit Server VM 11.0.15+0 on 11.0.15+0 +jit [arm64-darwin]
  • Operating system and platform (e.g. uname -a)
Darwin Mac-mini 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:29 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T8101 arm64

@headius
Copy link
Member Author

headius commented Aug 5, 2022

@k77ch7 Thank you! I agree, the impact seems minimal.

@headius headius merged commit 6fd96dd into jruby:master Aug 5, 2022
@headius headius deleted the check_interrupts_range_each branch August 5, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simple timeout script does not work
3 participants