December 2023 - ruby-core - ml.ruby-lang.org

[ruby-core:111342] [Ruby master Bug#19246] Rebuilding the loaded feature index much slower in Ruby 3.1

by thomthom (Thomas Thomassen)

Issue #19246 has been reported by thomthom (Thomas Thomassen). ---------------------------------------- Bug #19246: Rebuilding the loaded feature index much slower in Ruby 3.1 https://bugs.ruby-lang.org/issues/19246 * Author: thomthom (Thomas Thomassen) * Status: Open * Priority: Normal * Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN ---------------------------------------- Some background to this issue: (This is a case that is unconventional usage of Ruby, but I hope you bear with me.) We ship the Ruby interpreter with our desktop applications for plugin support in our application (SketchUp). One feature we have had since, at least 2006 (maybe earlier-hard to track history beyond that) is that we had a custom alternate `require` method: `Sketchup.require`. This allows the users of our API to load encrypted Ruby files. This originally used `rb_provide` to add the path to the encrypted file into the list of loaded feature. However, somewhere between Ruby 2.2 and 2.5 there was some string optimisations made and the function `rb_provide` would not use a copy of the string passed to it. Instead it just held on to a pointer reference. In our case that string came from user-land, being passed in from `Sketchup.require` and would eventually be garbage collected and cause access violation crashes. To work around that we changed our custom `Sketchup.require` to push to `$LOADED_FEATURES` directly. There was a small penalty to the index being rebuilt after that, but it was negligible. Recently we tried to upgrade the Ruby interpreter in our application from 2.7 to 3.1 and found a major performance reduction when using our `Sketchup.require. As in, a plugin that would load in half a second would now spend 30 seconds. From https://bugs.ruby-lang.org/issues/18452 it sounds like there is _some_ expected extra penalty due to changes in how the index is built. But should it really be this much? Example minimal repro to simulate the issue: ``` # frozen_string_literal: true require 'benchmark' iterations = 200 foo_files = iterations.times.map { |i| "#{__dir__}/tmp/foo-#{i}.rb" } foo_files.each { |f| File.write(f, "") } bar_files = iterations.times.map { |i| "#{__dir__}/tmp/bar-#{i}.rb" } bar_files.each { |f| File.write(f, "") } biz_files = iterations.times.map { |i| "#{__dir__}/tmp/biz-#{i}.rb" } biz_files.each { |f| File.write(f, "") } Benchmark.bm do |x| x.report('normal') { foo_files.each { |file| require file } } x.report('loaded_features') { foo_files.each { |file| require file $LOADED_FEATURES << "#{file}-fake.rb" } } x.report('normal again') { biz_files.each { |file| require file } } end ``` ``` C:\Users\Thomas\SourceTree\ruby-perf>ruby27.bat ruby 2.7.4p191 (2021-07-07 revision a21a3b7d23) [x64-mingw32] C:\Users\Thomas\SourceTree\ruby-perf>ruby test-require.rb user system total real normal 0.000000 0.031000 0.031000 ( 0.078483) loaded_features 0.015000 0.000000 0.015000 ( 0.038759) normal again 0.016000 0.032000 0.048000 ( 0.076940) ``` ``` C:\Users\Thomas\SourceTree\ruby-perf>ruby30.bat ruby 2.7.4p191 (2021-07-07 revision a21a3b7d23) [x64-mingw32] C:\Users\Thomas\SourceTree\ruby-perf>ruby test-require.rb user system total real normal 0.000000 0.031000 0.031000 ( 0.074733) loaded_features 0.032000 0.000000 0.032000 ( 0.038898) normal again 0.000000 0.047000 0.047000 ( 0.076343) ``` ``` C:\Users\Thomas\SourceTree\ruby-perf>ruby31.bat ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x64-mingw-ucrt] C:\Users\Thomas\SourceTree\ruby-perf>ruby test-require.rb user system total real normal 0.016000 0.031000 0.047000 ( 0.132633) loaded_features 1.969000 11.500000 13.469000 ( 18.395761) normal again 0.031000 0.125000 0.156000 ( 0.249130) ``` Right now we're exploring options to deal with this. Because the performance degradation is a blocker for us upgrading. We also have 16 years of plugins created by third party developer that makes it impossible for us to drop this feature. Some options as-is, none of which are ideal: 1. We revert to using `rb_provide` but ensure the string passed in is not owned by Ruby, instead building a list of strings that we keep around for the duration of the application process. The problem is that some of our plugin developers have on occasion released plugins that will touch `$LOADED_FEATURES`, and if such a plugin is installed on a user machine it might cause the application to become unresponsive for minutes. The other non-ideal issue with using `rb_provide` is that we're also using that in ways it wasn't really intended (from that I understand). And it's not an official API? 2. We create a separate way for our `Sketchup.require` to keep track of it's loaded features, but then that would diverge even more from the behaviour of `require`. Replicating `require` functionality is not trivial and would be prone to subtle errors and possible diverge. It also doesn't address our issue that there is code out there in existing plugins that touches `$LOADED_FEATURES`. (And it's not something we can just ask people to clean up. From previous experience old versions stick around for a long time and is very hard to purge from circulation.) I have two questions for the Ruby mantainers: 1. Would it be reasonable to see an API for adding/removing/checking `$LOADED_FEATURE` that would allow for a more ideal implementation of a custom `require` functionality? 2. Is the performance difference in rebuilding the loaded feature index really expected to be as high as what we're seeing? An increase of nearly 100 times? Is there something there that might be addressed to make the rebuild to be less expensive against? (This would really help to address our challenges with third party plugins occasionally touching the global.) -- https://bugs.ruby-lang.org/

4 months

9
21
0 0

[ruby-core:114590] [Ruby master Bug#19857] Eval coverage is reset after each `eval`.

by ioquatix (Samuel Williams)

Issue #19857 has been reported by ioquatix (Samuel Williams). ---------------------------------------- Bug #19857: Eval coverage is reset after each `eval`. https://bugs.ruby-lang.org/issues/19857 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- It seems like `eval` based coverage is reset every time eval is invoked. ```ruby #!/usr/bin/env ruby require 'coverage' def measure(flag) c = Class.new c.class_eval(<<~RUBY, "foo.rb", 1) def foo(flag) if flag puts "foo" else puts "bar" end end RUBY return c.new.foo(flag) end Coverage.start(lines: true, eval: true) # Depending on the order of these two operations, different computation is calculated, because the evaluation of the code is considered different, even if the content/path is the same. measure(false) measure(true) p Coverage.result ``` Further investigation is required. -- https://bugs.ruby-lang.org/

4 months

3
19
0 0

[ruby-core:115181] [Ruby master Bug#19975] ISeq#to_binary loses hidden local variable indices

by kddnewton (Kevin Newton)

Issue #19975 has been reported by kddnewton (Kevin Newton). ---------------------------------------- Bug #19975: ISeq#to_binary loses hidden local variable indices https://bugs.ruby-lang.org/issues/19975 * Author: kddnewton (Kevin Newton) * Status: Open * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- When you call `RubyVM::InstructionSequence#to_a`, you get hidden local variables as the index in the stack from the top: ``` c if (rb_id2str(lid)) { rb_ary_push(locals, ID2SYM(lid)); } else { /* hidden variable from id_internal() */ rb_ary_push(locals, ULONG2NUM(iseq_body->local_table_size-i+1)); } ``` ``` rb RubyVM::InstructionSequence.compile("for foo in bar; end").to_a[13][4][2][10] # => [2] ``` When you call `RubyVM::InstructionSequence#to_binary`, it dumps hidden local variables as 0: ``` c if (id == 0 || rb_id2name(id) == NULL) { return 0; } return ibf_dump_object(dump, rb_id2sym(id)); ``` When it reads that back in and then you call `to_a`, you get `:#arg_rest`: ``` ruby RubyVM::InstructionSequence.load_from_binary(RubyVM::InstructionSequence.compile("for foo in bar; end").to_binary).to_a[13][4][2][10] # => [:"#arg_rest"] ``` This means you end up not being able to consistently look at the locals. Instead, when reading back in from binary it could replace it with the index so that it matches up with the value before it is dumped to binary. Could we do that so that `#to_a` is consistent no matter where the iseq came from? -- https://bugs.ruby-lang.org/

4 months, 1 week

2
1
0 0

[ruby-core:115212] [Ruby master Bug#19983] Nested * seems incorrect

by Eregon (Benoit Daloze)

Issue #19983 has been reported by Eregon (Benoit Daloze). ---------------------------------------- Bug #19983: Nested * seems incorrect https://bugs.ruby-lang.org/issues/19983 * Author: Eregon (Benoit Daloze) * Status: Open * Priority: Normal * ruby -v: ruby 3.3.0dev (2023-10-30T09:27:06Z master 14fa5e39d7) [x86_64-linux] * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- ``` $ ruby -v -e 'def m(*); ->(*) { p(*) }; end; m(1).call(2)' ruby 3.3.0dev (2023-10-30T09:27:06Z master 14fa5e39d7) [x86_64-linux] 1 ``` But I would expect `2`. Much like: ``` $ ruby -e 'def m(a); ->(a) { p(a) }; end; m(1).call(2)' ruby 3.3.0dev (2023-10-30T09:27:06Z master 14fa5e39d7) [x86_64-linux] 2 ``` i.e. the inner variable should win. -- https://bugs.ruby-lang.org/

4 months, 1 week

4
8
0 0

[ruby-core:115845] [Ruby master Bug#20078] StringIO cannot be moved between Ractors

by forthoney (Seong-Heon Jung)

Issue #20078 has been reported by forthoney (Seong-Heon Jung). ---------------------------------------- Bug #20078: StringIO cannot be moved between Ractors https://bugs.ruby-lang.org/issues/20078 * Author: forthoney (Seong-Heon Jung) * Status: Open * Priority: Normal * ruby -v: 3.2 * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- The following code will raise an unusual Ractor error. ``` ruby require 'stringio' r = Ractor.new { loop { Ractor.receive } } Ractor.shareable?(StringIO.new) #=> false r.send(StringIO.new) # passing it via copy works r.send(StringIO.new, move: true) # <internal:ractor>:587:in `send': can not move StringIO object. (Ractor::Error) ``` I'm not 100% sure but I believe that this probably not the intended behavior considering * The error raised is a generic `Ractor::Error` rather than something specific like Ractor::MovedError or Ractor::IsolationError * It can be copied * [No documentation](https://docs.ruby-lang.org/en/master/ractor_md.html) exists for this error * I have yet to seen this happen on any other class instance * Typo (can not -> cannot) -- https://bugs.ruby-lang.org/

4 months, 1 week

3
6
0 0

[ruby-core:112091] [Ruby master Bug#19387] Issue with ObjectSpace.each_objects not returning IO objects after starting a ractor

by luke-gru (Luke Gruber)

Issue #19387 has been reported by luke-gru (Luke Gruber). ---------------------------------------- Bug #19387: Issue with ObjectSpace.each_objects not returning IO objects after starting a ractor https://bugs.ruby-lang.org/issues/19387 * Author: luke-gru (Luke Gruber) * Status: Open * Priority: Normal * Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- ```ruby r = Ractor.new do receive # block, the problem is not the termination of the ractor but the starting end ObjectSpace.each_object(IO) { |io| p io # we get no objects } ``` -- https://bugs.ruby-lang.org/

4 months, 1 week

3
5
0 0

[ruby-core:115736] [Ruby master Bug#20064] Inconsistent behavior between array splat *nil and hash splat **nil

by zeke (Zeke Gabrielse)

Issue #20064 has been reported by zeke (Zeke Gabrielse). ---------------------------------------- Bug #20064: Inconsistent behavior between array splat *nil and hash splat **nil https://bugs.ruby-lang.org/issues/20064 * Author: zeke (Zeke Gabrielse) * Status: Open * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- This has been discussed in #8507 and #9291 but both were closed because they lacked a clear use case. I think the following code should work, showing a clear use case: ```ruby invitation = if params.key?(:inviter_id) { invitation_attributes: params.slice(:inviter_id) } end User.create( email: 'john.doe(a)ruby.example', first_name: 'John', first_name: 'Doe', **invitation, ) ``` Per the previous discussions, this is because `*` uses explicit conversion to array (`to_a`, not `to_ary`), while `**` uses implicit conversion to hash (`to_hash`, not `to_h`). I find it confusing that you can splat nil into an array, but not nil into a hash. It would make sense for `**` to use explicit conversion. -- https://bugs.ruby-lang.org/

4 months, 1 week

4
4
0 0

[ruby-core:115491] [Ruby master Feature#20024] SyntaxError subclasses

by kddnewton (Kevin Newton)

Issue #20024 has been reported by kddnewton (Kevin Newton). ---------------------------------------- Feature #20024: SyntaxError subclasses https://bugs.ruby-lang.org/issues/20024 * Author: kddnewton (Kevin Newton) * Status: Open * Priority: Normal ---------------------------------------- There are many places around the Ruby ecosystem that handle syntax errors in different ways. Some provide highlighting, others provide recovery of some form, still more provide LSP metadata. In order to provide more rich information, most of them switch on the message of the error being returned, as in: https://github.com/ruby/irb/blob/f86d9dbe2fc05ed62332069a27f4aacc59ba9634/l… Within ruby/spec, specific error messages are required for these kinds of messages in order to support this implicit interface that syntax errors have a hidden type, which is only expressed through their message. For example: https://github.com/ruby/spec/blob/c3206f644325c026fc5b700f0ea75ce9bd2e9d02/… https://github.com/ruby/spec/blob/c3206f644325c026fc5b700f0ea75ce9bd2e9d02/… https://github.com/ruby/spec/blob/c3206f644325c026fc5b700f0ea75ce9bd2e9d02/… https://github.com/ruby/spec/blob/c3206f644325c026fc5b700f0ea75ce9bd2e9d02/… https://github.com/ruby/spec/blob/c3206f644325c026fc5b700f0ea75ce9bd2e9d02/… It's not clear from these specs or from the parser itself which error messages are permanent/guaranteed versus which are changeable. Either way, relying on the error message itself as opposed to the type of the error is brittle at best. I would like to suggest instead we implement subclasses on `SyntaxError` that would allow tools that depend on specific syntax errors to rescue those subclasses instead of parsing the message. In addition to alleviating the need to parse error messages with regex, this would also allow for the possibility that the error messages could change in the future without breaking external tooling. Allowing these to change would allow them to be potentially enhanced or changed by other tools - for example by providing recovery information or translating them. This is particularly important for Prism since we are getting down to individual spec failures and some of the failures are related to the fact that we have messages like `"Numbered parameter is already used in outer scope"` where the spec requires `/numbered parameter is already used in/`. Even this case-sensitivity is causing failures, which seems like we're testing the wrong thing. -- https://bugs.ruby-lang.org/

4 months, 1 week

9
16
0 0

[ruby-core:113788] [Ruby master Bug#19717] `ConditionVariable#signal` is not fair when the wakeup is consistently spurious.

by ioquatix (Samuel Williams)

Issue #19717 has been reported by ioquatix (Samuel Williams). ---------------------------------------- Bug #19717: `ConditionVariable#signal` is not fair when the wakeup is consistently spurious. https://bugs.ruby-lang.org/issues/19717 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- For background, see this issue <https://github.com/socketry/async/issues/99>. It looks like `ConditionVariable#signal` is not fair, if the calling thread immediately reacquires the resource. I've given a detailed reproduction here as it's non-trivial: <https://github.com/ioquatix/ruby-condition-variable-timeout>. Because the spurious wakeup occurs, the thread is pushed to the back of the waitq, which means any other waiting thread will acquire the resource, and that thread will perpetually be at the back of the queue. I believe the solution is to change `ConditionVarialbe#signal` should only remove the thread from the waitq if it's possible to acquire the lock. Otherwise it should be left in place, so that the order is retained, this should result in fair scheduling. -- https://bugs.ruby-lang.org/

4 months, 1 week

3
12
0 0

[ruby-core:115684] [Ruby master Feature#20057] Change behaviour of rb_register_postponed_job for Ruby 3.3

by kjtsanaktsidis (KJ Tsanaktsidis)

Issue #20057 has been reported by kjtsanaktsidis (KJ Tsanaktsidis). ---------------------------------------- Feature #20057: Change behaviour of rb_register_postponed_job for Ruby 3.3 https://bugs.ruby-lang.org/issues/20057 * Author: kjtsanaktsidis (KJ Tsanaktsidis) * Status: Open * Priority: Normal ---------------------------------------- This ticket is to discuss some changes to `rb_register_postponed_job` that @ko1 and myself propose to make for Ruby 3.3. The motivation for this work is to fix a bug in the current implementation, which can cause the registered functions to be called with the wrong `data` argument (https://bugs.ruby-lang.org/issues/19991). There's a long discussion on the associated PR (https://github.com/ruby/ruby/pull/9041) but in the end we came to the conclusion that the best way to fix this bug involved actually changing the current semantics of `rb_register_postponed_job`. I'm opening this issue to get feedback on this approach and to see if anybody knows of a reason why we should not release this for Ruby 3.3. ## Current behaviour in Ruby 3.2 Currently, Ruby has two functions for interacting with postponed jobs. These jobs can be enqueued from anywhere (including signal handlers), and will be executed next time Ruby checks for `RUBY_VM_CHECK_INTS()`. * `rb_postponed_job_register(func, data)`: Schedules `func(data)` to be executed the next time `RUBY_VM_CHECK_INTS` is checked. * `rb_postponed_job_register_once(func, data)`: Works like `rb_postponed_job_register`, _except_ if `func` is already scheduled to be executed (either with this `data` or with different `data`), in which case it does nothing. The postponed jobs are stored in a fixed sized array (of length 1024), so it's possible that enqueuing them could fail if the buffer is full. In this case, they signal this by returning `0` (otherwise, they return `1` for successful enqueue or `2` because `rb_postponed_job_register_once` did nothing because `func` was already in the queue). Unfortunately, as I mentioned before, the implementation of these functions are subject to a race condition because `func` and `data` are not written into the postponed job buffer together atomically (they are two separate variables and CPUs tend not to have double-word atomic instructions). Again, see https://bugs.ruby-lang.org/issues/19991 for the full details. ## What we have done Whilst working on this issue, we had a look at all of the in-the-wild usages of these APIs on rubygems. The only real usage of these APIs is for profiling tools, and the following was true for essentially all of them: * Each gem only is registering a single callback function, * Almost all of the usages either make no use of the `data` argument at all, or pass some kind of never-changing global context into it. * There are only a very small handful of gems using these APIs at all Thus, we concluded that the current behaviour of allowing scheduling and execution of arbitrary `(func, data)` pairs is actually not really needed. Instead, we could offer a more limited API which would meet the needs of all current users, whilst making it easy to avoid the race conditions in the current implementation. The new API is as follows: * `rb_postponed_job_preregister(func, data)`: This function registers `func`/`data` into a small, fixed-size table, and return a handle to this registration. Subsequent calls to this function with the same `func` will return the same handle, and overwrite the `data` with new data if it is different. The size of the table is 32 entries on most systems, which is still enough to use literally every gem on rubygems that actually uses these APIs at the same time. The intention is that libraries would call this function in their initialization routines, storing the handle for later. * `rb_postponed_job_trigger(handle)`: This function takes the handle from `rb_postponed_job_preregister` and schedules it for execution the next time `RUBY_VM_CHECK_INTS` is called. If the handle is already scheduled, this will not cause it to be scheduled twice; each `func` can only be called a maximum of one time for each call to `RUBY_VM_CHECK_INTS`, essentially. All of the usages of the old `rb_postponed_job_register{,_once}` functions in the Ruby tree have been replaced by calls to the above two functions, and these two old functions have been marked with the deprecated attribute. They have also been re-implemented in terms of the new functions; both `rb_postponed_job_register` and `rb_postponed_job_register_once` are now both equivalent to `rb_postponed_job_trigger(rb_postponed_job_prereigster(func, data))`. This means that: * `rb_postponed_job_register` now works like `rb_postponed_job_register_once` i.e. `func` can only be executed one time per `RUBY_VM_CHECK_INTS`, no matter how many times it is registered * They are also called with the _last_ `data` to be registered, not the first (which is how `rb_postponed_job_register_once` previously worked) I verified that stackprof still builds & works correctly with the new implementation of `rb_postponed_job_register`. ## What else we tried I tried a couple of things to keep the current semantics of `rb_postponed_job_register{,_once}` intact, without introducing new APIs. * First, I tried protecting postponed job buffer by masking signals around the critical section & using a POSIX semaphore instead of a pthread mutex: https://github.com/ruby/ruby/pull/8856. However, there was a concern that this would be too slow (since `RUBY_VM_CHECK_INTS` is called very often, and both the semaphore and the signal mask require calling into the kernel). * Then, I implemented a lock-free ringbuffer to store the postponed job queue: https://github.com/ruby/ruby/compare/master...KJTsanaktsidis:ruby:old_circu…. However, the concern with this implementation was that it was too complex. ## Ruby 3.3 As of right now, we have merged these changes (from https://github.com/ruby/ruby/pull/9041), and @ko1 plans for them to go out in 3.3-rc1. The point of opening this issue is to ask: does anybody foresee any problem with our approach? -- https://bugs.ruby-lang.org/

4 months, 1 week

3
7
0 0

2024

2023

2022

ruby-core December 2023