Make global variables thread-safe in the extension

[andrykonchin]

There are global variables in the native extension what make it not thread-safe on TruffleRuby or any other alternative Ruby implementation that doesn't have GIL.

The issue was reported in oracle/truffleruby#3625.

The approach is to declare these global variables as thread local either with C11's _Thread_local or GCC-specific __thread declaration.

[andrykonchin]

This logic is borrowed from the thread_pthread.h file. On Ruby master it was removed in ruby/ruby@f99af43 and RB_THREAD_LOCAL_SPECIFIER is declared instead.

So on Ruby 3.4 we rely on the available out of the box RB_THREAD_LOCAL_SPECIFIER and on earlier versions the previous logic is used. This way thread-local declarations should work on both existing Ruby versions and Ruby 3.4.

[andrykonchin]

I didn't analyse thoroughly which global variables are modified and which aren't so updated declaration of all the static global variables.

[andrykonchin]

The issue is easily reproducible on TruffleRuby with the following code:

require 'nkf'

threads = []

10.times do
  th = Thread.new do
    1000.times { NKF.guess("Hello world") }
  end

  threads << th
end

threads.each(&:join)

It should be run with a --cexts-lock=false option to disable the global lock for native extensions:

$ ruby -Ilib --experimental-options --cexts-lock=false test.rb

When it's run with nkf without the fix the issue is leads to the following error:

#<Thread:0x288 test.rb:6 run> terminated with exception (report_on_exception is true):
string.c:187:in `rb_str_resize': TruffleRuby doesn't have a case for the org.truffleruby.cext.CExtNodesFactory$RbStrResizeNodeFactory$RbStrResizeNodeGen node with values of type java.lang.Boolean=false java.lang.Integer=0 (TypeError)
	from org.truffleruby.cext.CExtNodesFactory$RbStrResizeNodeFactory$RbStrResizeNodeGen.executeAndSpecialize(CExtNodesFactory.java:3944)
	from org.truffleruby.cext.CExtNodesFactory$RbStrResizeNodeFactory$RbStrResizeNodeGen.execute(CExtNodesFactory.java:3922)
	from org.truffleruby.language.RubyCoreMethodRootNode.execute(RubyCoreMethodRootNode.java:58)
	from string.c:187:in `rb_str_resize'
	from /Users/andrykonchin/projects/truffleruby-ws/truffleruby/mxbuild/truffleruby-jvm/lib/truffle/truffle/cext_ruby.rb:38:in `guess'
	from test.rb:7:in `block (3 levels) in <main>'
	from <internal:core> core/integer.rb:154:in `times'
	from test.rb:7:in `block (2 levels) in <main>'

So the rb_str_resize function is called with false instead of a String argument (that's rb_str_resize(false, 0)). I suppose false is a value of not initialised VALUE result global variable.

[headius]

Other than pinning this code to C11+, it will also incur a performance penalty for most accesses:

https://david-grs.github.io/tls_performance_overhead_cost_linux/

Because the nkf extension is loaded as a dynamic library, most optimizations of the TLS won't work and it will revert to a slow-path thread-local lookup (2x slower in the above example). I have to wonder if it wouldn't make more sense to just move these variables into a proper CRuby thread-local, which would avoid the C11 requirement, fix the issue, and be more explicit in form.

I did not research what the impact is on Windows or platforms other than those covered in the above article.

JRuby doesn't use this code, so it doesn't matter to us, but I happened to stumble on this issue while reviewing stdlib C exts today. I look forward to the day this extension just goes away. 😀

[eregon]

I have to wonder if it wouldn't make more sense to just move these variables into a proper CRuby thread-local, which would avoid the C11 requirement, fix the issue, and be more explicit in form.

ext/nkf/nkf-utf8/nkf.c is as I understand upstream code so it seems best to avoid modifying it too much.
Also currently that file doesn't use any Ruby-specific things.

Regarding C11 I would expect __thread is supported since a long time in gcc & clang, so I expect it to work on any non-ancient compiler (and the logic is the same in CRuby BTW).

Regarding the speed, Ractor uses TLS too (and there can be a dynamic libruby.so) so I think it's an insignificant overhead, especially for NKF.

I'm not sure why the upstream NKF authors chose to pass state in global variables and not parameters, but that's anyway not optimal for performance, even before this change.

[mohamedhafez]

Would it be possible to mark the C-extension with

#ifdef HAVE_RB_EXT_RACTOR_SAFE
   rb_ext_ractor_safe(true);
#endif

since this PR makes it threadsafe/Ractor-safe?

[eregon]

since this PR makes it threadsafe/Ractor-safe?

This PR makes it thread-safe but not Ractor-safe, because there is still global state per thread, and multiple Ractors can run on the same native thread with RUBY_MN_THREADS=1.

[eregon]

What is the upstream of ext/nkf/nkf-utf8/nkf.c ? Is it https://github.com/nurse/nkf/blob/master/nkf.c ?