[ruby-core:122238] [Ruby Bug#21362] Namespace: Inline method caches poisoned with builtins

[ruby-core:122238] [Ruby Bug#21362] Namespace: Inline method caches poisoned with builtins

[jhawthorn (John Hawthorn) via ruby-core]

Issue #21362 has been reported by jhawthorn (John Hawthorn).

----------------------------------------
Bug #21362: Namespace: Inline method caches poisoned with builtins
https://bugs.ruby-lang.org/issues/21362

* Author: jhawthorn (John Hawthorn)
* Status: Open
* Backport: 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN
----------------------------------------
``` ruby
File.write("/tmp/ntest.rb", <<~'RUBY')
class Integer
  def succ = self + 2
end

module Test
  def self.run = 10.times.to_a
end
RUBY

module Test
  def self.run = 10.times.to_a
end

ns = Namespace.new
ns.require("/tmp/ntest.rb")

p namespaced: ns::Test.run
p main: Test.run
```

```
RUBY_NAMESPACE=1 ruby test_namespace_succ.rb
ruby: warning: Namespace is experimental, and the behavior may change in the future!
See doc/namespace.md for known issues, etc.
{namespaced: [0, 2, 4, 6, 8]}
{main: [0, 2, 4, 6, 8]}
```

What's happening here is that we have a number of "builtin" Ruby files which are loaded during the VM boot. The instruction sequences and method definitions from these files end up as part of the "root" namespace. However this iseq can still include inline caches, and in this example we see the cache poisoned by being run in a namespace with succ redefined. I was surprised by this, I expected it to call using the "root" namespace.

It's definitely a strange edge-case to redefine `succ`, but the namespace semantics here are important because how this is defined impacts how useful builtins can be:

- If builtins inherit their namespace from the caller, then they cannot use inline caches (or inline caches must always check for namespace validity) and also cannot be JIT compiled (unless the JIT also checks for namespace validity, and given the complexity of rb_current_namespace, it would be very undesirable). This is very bad as converting C code to builtin Ruby has been important for getting better performance.

- If builtins make calls using the "root" namespace, inline caching and the JIT should work (actually there may be advantages to both since the namespace is mostly immutable). However this will limit what we are able to move from C to Ruby, as it wouldn't be possible to have the same semantics as C methods (where the caller's namespace is used).




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:122239] [Ruby Bug#21362] Namespace: Inline method caches poisoned with builtins

[Eregon (Benoit Daloze) via ruby-core]

Issue #21362 has been updated by Eregon (Benoit Daloze).


Yeah such builtin/core Ruby files clearly need to be loaded and executed in the root namespace from a performance POV, otherwise it means no inline caches (my initial worry in https://bugs.ruby-lang.org/issues/21311#note-15).
It's quite a weird bug this isn't already the case, as those files are AFAIK already loaded in the root namespace, but somehow it doesn't "remember" when calling the methods defined there?

Though as you say then it means any monkey patches to core classes would not be visible to core methods defined in Ruby which could call methods on passed arguments, e.g. if defining a custom `hash` or coercion method or so on some core class.
I suspect CRuby has few core methods defined in Ruby affected by that, but it would increase as more core methods are defined in Ruby.
TruffleRuby which has most core methods defined in Ruby would probably see issues with these namespace semantics sooner, if it implements namespaces (I think not likely as sub-interpreters seem superior in every way and already supported).

----------------------------------------
Bug #21362: Namespace: Inline method caches poisoned with builtins
https://bugs.ruby-lang.org/issues/21362#change-113388

* Author: jhawthorn (John Hawthorn)
* Status: Open
* Backport: 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN
----------------------------------------
``` ruby
File.write("/tmp/ntest.rb", <<~'RUBY')
class Integer
  def succ = self + 2
end

module Test
  def self.run = 10.times.to_a
end
RUBY

module Test
  def self.run = 10.times.to_a
end

ns = Namespace.new
ns.require("/tmp/ntest.rb")

p namespaced: ns::Test.run
p main: Test.run
```

```
RUBY_NAMESPACE=1 ruby test_namespace_succ.rb
ruby: warning: Namespace is experimental, and the behavior may change in the future!
See doc/namespace.md for known issues, etc.
{namespaced: [0, 2, 4, 6, 8]}
{main: [0, 2, 4, 6, 8]}
```

What's happening here is that we have a number of "builtin" Ruby files which are loaded during the VM boot. The instruction sequences and method definitions from these files end up as part of the "root" namespace. However this iseq can still include inline caches, and in this example we see the cache poisoned by being run in a namespace with succ redefined. I was surprised by this, I expected it to call using the "root" namespace.

It's definitely a strange edge-case to redefine `succ`, but the namespace semantics here are important because how this is defined impacts how useful builtins can be:

- If builtins inherit their namespace from the caller, then they cannot use inline caches (or inline caches must always check for namespace validity) and also cannot be JIT compiled (unless the JIT also checks for namespace validity, and given the complexity of rb_current_namespace, it would be very undesirable). This is very bad as converting C code to builtin Ruby has been important for getting better performance.

- If builtins make calls using the "root" namespace, inline caching and the JIT should work (actually there may be advantages to both since the namespace is mostly immutable). However this will limit what we are able to move from C to Ruby, as it wouldn't be possible to have the same semantics as C methods (where the caller's namespace is used).




-- 
https://bugs.ruby-lang.org/
 ______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/

[ruby-core:122422] [Ruby Bug#21362] Namespace: Inline method caches poisoned with builtins

[matz (Yukihiro Matsumoto) via ruby-core]

Issue #21362 has been updated by matz (Yukihiro Matsumoto).


Cache poisoning itself seems to be a bug to be fixed.

But it should be pointed out that whether a method is replaced when it is called from another namespace is an important design choice, and is called local rebinding. We believe that it is better not to do local rebinding (for Ruby Namespace), just like Refinement does not.

Matz.


----------------------------------------
Bug #21362: Namespace: Inline method caches poisoned with builtins
https://bugs.ruby-lang.org/issues/21362#change-113602

* Author: jhawthorn (John Hawthorn)
* Status: Open
* Backport: 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN
----------------------------------------
``` ruby
File.write("/tmp/ntest.rb", <<~'RUBY')
class Integer
  def succ = self + 2
end

module Test
  def self.run = 10.times.to_a
end
RUBY

module Test
  def self.run = 10.times.to_a
end

ns = Namespace.new
ns.require("/tmp/ntest.rb")

p namespaced: ns::Test.run
p main: Test.run
```

```
RUBY_NAMESPACE=1 ruby test_namespace_succ.rb
ruby: warning: Namespace is experimental, and the behavior may change in the future!
See doc/namespace.md for known issues, etc.
{namespaced: [0, 2, 4, 6, 8]}
{main: [0, 2, 4, 6, 8]}
```

What's happening here is that we have a number of "builtin" Ruby files which are loaded during the VM boot. The instruction sequences and method definitions from these files end up as part of the "root" namespace. However this iseq can still include inline caches, and in this example we see the cache poisoned by being run in a namespace with succ redefined. I was surprised by this, I expected it to call using the "root" namespace.

It's definitely a strange edge-case to redefine `succ`, but the namespace semantics here are important because how this is defined impacts how useful builtins can be:

- If builtins inherit their namespace from the caller, then they cannot use inline caches (or inline caches must always check for namespace validity) and also cannot be JIT compiled (unless the JIT also checks for namespace validity, and given the complexity of rb_current_namespace, it would be very undesirable). This is very bad as converting C code to builtin Ruby has been important for getting better performance.

- If builtins make calls using the "root" namespace, inline caching and the JIT should work (actually there may be advantages to both since the namespace is mostly immutable). However this will limit what we are able to move from C to Ruby, as it wouldn't be possible to have the same semantics as C methods (where the caller's namespace is used).




-- 
https://bugs.ruby-lang.org/
______________________________________________
 ruby-core mailing list -- ruby-core@ml.ruby-lang.org
 To unsubscribe send an email to ruby-core-leave@ml.ruby-lang.org
 ruby-core info -- https://ml.ruby-lang.org/mailman3/lists/ruby-core.ml.ruby-lang.org/