Skip to content

Conversation

tagomoris
Copy link
Contributor

@tagomoris tagomoris commented May 1, 2025

This change is to implement "Namespace on read" proposed by Feature #21311.

TODO:

  • Identify the cause and fix CI failures
  • Move prime classext readable/writable flags to RBasic flags (or abort this plan)
  • Rename RCLASS_EXT macro to prevent using it wrongly in the past way
  • Check review comments and fix code
  • Get a very basic benchmark to check performance regression
  • Show [EXPERIMENTAL] warning when namespace is enabled
  • Remove methods for debugging (rb_class_debug_dump_*)
  • Add a minimum doc, containing synopsis, known problems/limitations, list to be done, etc

@tagomoris tagomoris mentioned this pull request May 1, 2025
22 tasks

This comment has been minimized.

@ko1 ko1 added the Playground Experimental: Post link to try Ruby with PR changes label May 2, 2025
Copy link

github-actions bot commented May 2, 2025

Try on Playground: https://ruby.github.io/play-ruby?run=14956371761
This is an automated comment by pr-playground.yml workflow.

@tagomoris tagomoris force-pushed the namespace-on-read-classext branch 3 times, most recently from bdc640f to e09126f Compare May 2, 2025 03:58
@tagomoris tagomoris force-pushed the namespace-on-read-classext branch 2 times, most recently from ddeb57d to e62e3e5 Compare May 6, 2025 09:04
@ko1 ko1 added Playground Experimental: Post link to try Ruby with PR changes and removed Playground Experimental: Post link to try Ruby with PR changes labels May 6, 2025
@fxn
Copy link
Contributor

fxn commented May 6, 2025

A curiosity. I have

# test.rb
p Object.constants

ns = Namespace.new
ns.require_relative 'foo'

and

# foo.rb
p Object.constants

If I run make run and compare, the main namespace has one extra constant: :CROSS_COMPILING.

My understanding is that this implies said top-level constant must not belong to the root namespace and it is created in the main user namespace. But, test.rb is not creating such constant. Which is the definition of root/user namespace?

@fxn
Copy link
Contributor

fxn commented May 6, 2025

I reported a segfault in this comment, but it was produced by my script trying to use pp with miniruby. Using puts I can see the location of CROSS_COMPILING: build/arm64-darwin24-fake.rb:15.

@tagomoris
Copy link
Contributor Author

@fxn I ran the same script on ruby (and miniruby) and I got :CROSS_COMPILING in both output lines.

$ RUBY_NAMESPACE=1 build/exe/ruby -e 'p Object.constants.sort; ns = Namespace.new; ns.require_relative("foo")'
[:ARGF, :ARGV, :ArgumentError, :Array, :BasicObject, :Binding, :CROSS_COMPILING, :Class, # (snip)
[:ARGF, :ARGV, :ArgumentError, :Array, :BasicObject, :Binding, :CROSS_COMPILING, :Class, # (snip)
$ cat foo.rb 
p Object.constants.sort

On the other hand, when using miniruby, there were no :CROSS_COMPILING in both output lines.

$ RUBY_NAMESPACE=1 build/miniruby -e 'p Object.constants.sort; ns = Namespace.new; ns.require_relative("foo")'
[:ARGF, :ARGV, :ArgumentError, :Array, :BasicObject, :Binding, :Class, :ClosedQueueError, # (snip)
[:ARGF, :ARGV, :ArgumentError, :Array, :BasicObject, :Binding, :Class, :ClosedQueueError, # (snip)

:CROSS_COMPILING is set in rbconfig.rb for ruby, and in #{archname}-fake.rb for miniruby.
I just can imagine, does something (library?) require those files accidentally in your environment?

@tagomoris tagomoris force-pushed the namespace-on-read-classext branch 2 times, most recently from cfaf65e to 399908b Compare May 8, 2025 15:49

### Top level constants

Usually, top level constants are defined as constants of `Object`. In namespaces, top level constants are constants of `Object` in the namespace. And the namespace object `ns`'s constants are strictly equal to constants of `Object`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the namespace object ns's constants are strictly equal to constants of Object.

How is this implemented? Is the constants table shared between both the Namespace and that namespace's Object?
That seems quite dangerous implementation-wise as it might have far-reaching implications on constant lookup caching and invalidation.
I think it would be better to preserve the invariant that a constant table is only used for one Module, and implement ns::Foo another way, e.g. via const_missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this implemented? Is the constants table shared between both the Namespace and that namespace's Object?

Correct.
As far as I checked (and CI checked), it's working correctly. But I'll rethink if we find it'll not work well.


#### Namespace#inspect

Currently, `Namespace#inspect` returns values like `"#<Namespace:0x00000001083a5660>"`. This results in the very redundant and poorly visible classpath outside the namespace.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not only outside the namespace but also inside, so "always":

$ RUBY_NAMESPACE=1 ruby -ve 'ns = Namespace.new; File.write "ns.rb", "class C; end; p C"; ns.require "./ns"'    
ruby 3.5.0dev (2025-05-10T07:50:29Z namespace-on-read-.. bd4f57f96b) +PRISM [x86_64-linux]
ruby: warning: Namespace is experimental, and the behavior may change in the future!
See doc/namespace.md for know issues, etc.
#<Namespace:0x00007fd81ce67598>::C

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your point is correct.
I'll fix the classpath inside the namespace sooner, and it's noted in TODOs. So the statement here is to describe the situation after the fix.

Return classpath and nesting without the namespace prefix in the namespace itself #21316, #21318

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, the "Discussions" section is to make discussions after merging this change, and I'm not going to make comments in this pull-requests on those points.
Thank you for making the comments.


Testing namespace features needs to create files to be loaded in namespaces. It's not easy nor casual.

If `Namespace` class has an instance method `#eval` to evaluate code in the namespace, it can be helpful.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was checking if this exists, I think it would be quite convenient.
That might also mean there is less need for ns::C and ns.eval('C') might be enough.


#### More builtin methods written in Ruby

If namespace is enabled by default, builtin methods can be written in Ruby because it can't be overridden by users' monkey patches. Builtin Ruby methods can be JIT-ed, and it could bring performance reward.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can't be overridden by users' monkey patches

It would be good to expand this to explain what it would solve concretely with examples.
In https://bugs.ruby-lang.org/issues/21311#note-36 I wrote that method monkey patches are usually dealt with Primitives.
Maybe Namespace would make this nicer/less necessary?
Namespace I guess could make Primitive.is_a?(obj, String) safer in the case String would be re-assigned by some code (that's very unlikely though).


It is a breaking change.

It's an option to change the behavior of methods in the root namespace to refer to definitions in user namespaces. But if we do so, that means we can't proceed with "More builtin methods written in Ruby".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an option to change the behavior of methods in the root namespace to refer to definitions in user namespaces.

How would that work though, e.g. if multiple namespaces have different Hash#each definitions?
You would always consider the methods of the Namespace.current also in calls inside builtin methods I guess.
That then becomes a problem for method lookup inline caching.


#### Expose top level methods as a method of the namespace object

Currently, top level methods in namespaces are not accessible from outside of the namespace. But there might be a use case to call other namespace's top level methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Namespace#eval would solve that


NOTE: "builtin" namespace is a different one from the "builtin" namespace in the current implementation

Currently, the single "root" namespace is the source of classext CoW. And also, the "root" namespace can load additional files after starting main script evaluation by calling methods which contain lines like `require "openssl"`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root namespace, assuming it contains only the core library/builtin classes should have no method doing any require, isn't it?


Now the classext CoW requires RClass and `rb_classext_t` to fallback its instance variable management from Object Shapes to the traditional `st_table`. It may have a performance penalty.

If we can apply Object Shapes on `rb_classext_t` instead of `RClass`, per-namespace classext can have its own shapes, and it may be able to avoid the performance penalty.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though that probably means an overhead for the non-Namespace case as the Shape is at least one more indirection away from the Class object.

@tagomoris tagomoris force-pushed the namespace-on-read-classext branch 2 times, most recently from 230babd to b05960c Compare May 10, 2025 15:47
tagomoris and others added 22 commits May 11, 2025 18:27
```
namespace.c: In function ‘rb_namespace_available’:
namespace.c:55:1: warning: old-style function definition [-Wold-style-definition]
   55 | rb_namespace_available()
      | ^~~~~~~~~~~~~~~~~~~~~~
```
```
internal/class.h:158:20: warning: ‘RCLASS_SET_CLASSEXT_TABLE’ declared ‘static’ but never defined [-Wunused-function]
  158 | static inline void RCLASS_SET_CLASSEXT_TABLE(VALUE obj, st_table *tbl);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~
internal/class.h:271:20: warning: ‘RCLASS_WRITE_SUBCLASSES’ declared ‘static’ but never defined [-Wunused-function]
  271 | static inline void RCLASS_WRITE_SUBCLASSES(VALUE klass, rb_subclass_anchor_t *anchor);
      |                    ^~~~~~~~~~~~~~~~~~~~~~~
```
```
  /github/workspace/src/proc.c:2023:65: error: format specifies type 'void *' but the argument has type 'const rb_namespace_t *' (aka 'const struct rb_namespace_struct *') [-Werror,-Wformat-pedantic]
   2023 |     rb_bug("Unexpected namespace on the method definition: %p", ns);
        |                                                            ~~   ^~
```
`rb_define_private_method` performs strict type checking on the function
pointer. As a result, we cannot pass the function a generic signature.

```
  /github/workspace/src/namespace.c:1097:72: note: expected 'VALUE (*)(void)' {aka 'long unsigned int (*)(void)'} but argument is of type 'VALUE (*)(int,  VALUE *, VALUE)' {aka 'long unsigned int (*)(int,  long unsigned int *, long unsigned int)'}
   1097 | namespace_define_loader_method(VALUE module, const char *name, VALUE (*func)(ANYARGS), int argc)
        |                                                                ~~~~~~~~^~~~~~~~~~~~~~
```

This commit defines the method directly to avoid the mismatch error.
To make RClass size smaller, move flags of prime classext readable/writable to:
 readable - use ns_classext_tbl is NULL or not (if NULL, it's readable)
 writable - use FL_USER2 of RBasic flags
…rongly

The macro RCLASS_EXT() accesses the prime classext directly, but it can be
valid only in a limited situation when namespace is enabled.
So, to prevent using RCLASS_EXT() in the wrong way, rename the macro and
let the developer check it is ok to access the prime classext or not.
```
/opt/rubies/head-namespaces/bin/ruby(sigsegv+0x84) [0x104e897e8]
/usr/lib/system/libsystem_platform.dylib(_sigtramp+0x38) [0x18de56de4]
/opt/rubies/head-namespaces/bin/ruby(object_id+0x80) [0x104d7d420]
/opt/rubies/head-namespaces/bin/ruby(object_id+0x80) [0x104d7d420]
/opt/rubies/head-namespaces/bin/ruby(rb_initialize_main_namespace+0xe4) [0x104ddaa20]
/opt/rubies/head-namespaces/bin/ruby(ruby_opt_init+0x120) [0x104e7f524]
/opt/rubies/head-namespaces/bin/ruby(ruby_process_options+0x1370) [0x104e7e31c]
/opt/rubies/head-namespaces/bin/ruby(ruby_options+0xb0) [0x104d69844]
/opt/rubies/head-namespaces/bin/ruby(main+0x64) [0x104ca8d54]
```

I'm not too sure why `rb_obj_id` crashes, but I suspect it's called too
early. Either way I don't think generating an object_id for namespaces
is a good idea. If all we need is a unique number we can do that
for much cheaper.
@tagomoris tagomoris force-pushed the namespace-on-read-classext branch from bf65b78 to 469297a Compare May 11, 2025 09:27
byroot and others added 3 commits May 11, 2025 12:02
If we're referencing the namespace object we have to mark it.
…autoload-marks

Add missing gc_mark in `autoload_const_mark`
…tion on this object.

#7
RUBY_TYPED_FREE_IMMEDIATELY can be added because namespace_entry_free does no IO nor
things to block.
@tagomoris tagomoris merged commit b94df81 into ruby:master May 11, 2025
83 of 84 checks passed
st_data_t classext_ptr;
st_table *classext_tbl = RCLASS_CLASSEXT_TBL(obj);
if (classext_tbl) {
if (rb_st_lookup(classext_tbl, (st_data_t)ns->ns_object, &classext_ptr)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like this is safe to do without holding the VM lock. (The VM lock is held on write so it must also be held on read, otherwise the lock has no benefit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Playground Experimental: Post link to try Ruby with PR changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants