Extract out usages of Ruby IDs #2175

amomchilov · 2024-12-18T00:53:35Z

This PR extracts out an rbs_constant_pool interface, which mimics the pm_constant_pool from Prism.

I use it to wrap all usages of ID as location child keys, instead of directly depending Ruby APIs like rb_intern(), ID2SYM, SYM2ID, etc. This includes the keys used to identify the children in a rb_loc, and the keys used in the parser's typevar tables.

For now, this is just a dummy implementation which delegates to those same Ruby APIs, but will eventually be replaced with Prism's real logic, which doesn't depend on ruby.h at all.

For easier reviewing, have a look at each commit separately.

amomchilov · 2024-12-18T00:59:33Z

ext/rbs_extension/location.c

@@ -221,9 +239,9 @@ static VALUE rbs_new_location_from_loc_range(VALUE buffer, rbs_loc_range rg) {
 static VALUE location_aref(VALUE self, VALUE name) {
  rbs_loc *loc = rbs_check_location(self);

-  ID id = SYM2ID(name);


I think this used to leak memory, since SYM2ID would allocate a static symbol.

amomchilov · 2024-12-18T05:19:27Z

ext/rbs_extension/parser.c

@@ -1023,8 +1031,13 @@ static VALUE parse_simple(parserstate *state) {
    return parse_symbol(state);
  }
  case tUIDENT: {
-    ID name = INTERN_TOKEN(state, state->current_token);


This INTERN_TOKEN macro was calling rb_intern3(), which was slowly leaking memory by creating static ("immortal") symbols.

The replacement rbs_constant_pool_find() function uses rb_check_id_cstr() instead, which checks for a match without intern a new symbol if there wasn't one.

Wondering if you plan to introduce another helper function instead of writing the token manipulation code everywhere in the codebase? (maybe later.)

Indeed, there's a lot of repetition in how the current token is handled. I'll add a helper eventually, it's up in our WIP branch :)

amomchilov · 2024-12-18T05:28:39Z

include/rbs/util/rbs_constant_pool.h

+
+/** The overall constant pool, which stores constants found while parsing. */
+typedef struct {
+    void *dummy; // Workaround for structs not being allowed to be empty.


Eventually, this will be replaced with the real storage that will store the constant pool, instead of the one built into the Ruby VM.

amomchilov · 2025-01-06T18:35:17Z

src/util/rbs_constant_pool.c

+    _Static_assert(
+        __builtin_types_compatible_p(ID, rbs_constant_id_t),
+        "rbs_constant_id_t must be the same as a Ruby ID for now."
+    );
+
+    _Static_assert(
+        __builtin_types_compatible_p(VALUE, rbs_constant_t),
+        "rbs_constant_t must be the same as a Ruby Symbol for now."
+    );


These assertions confirm that our temporary definitions of rbs_constant_id_t and rbs_constant_t are memory-compatible with ID and VALUE (Symbol), so that we can safely cast back/forth between them. They'll go away once I add in the pure C implementation of the constant pool.

soutaro · 2025-01-08T06:04:01Z

@amomchilov Thanks!

Let me confirm that: all other usage of Ruby symbols will finally be replaced to rbs_constant_pool to make the implementation independent to MRI C API, right?

(I'm a bit confused that some code continue using INTERN_TOKEN and other Ruby APIs instead of the constant pool.)

soutaro · 2025-01-08T06:08:39Z

ext/rbs_extension/main.c

  rbs__init_constants();
  rbs__init_location();
  rbs__init_parser();
+
+  rbs_constant_pool_init(RBS_GLOBAL_CONSTANT_POOL, 0);
+  ruby_vm_at_exit(Deinit_rbs_extension);


I'm not very sure if we need the deinit function. How can a Ruby vm exit without process termination? (Maybe there’s a reason I’m not aware of…)

~~Embedding CRuby?~~

I just added this to minimize the amount of leaked memory at the time the process terminates, so I could more easily find other leaks with tooling like valgrind.

We can remove it and just rely on process termination to clean up everything, but that has the downside of adding noise to any memory-related tooling. Perhaps we could add a DEBUG flag, and only do the cleanup when it's true?

How can a Ruby vm exit without process termination?

Not that I know of, but it seemed like the exact API for this (ther's also atexit())

I could more easily find other leaks with tooling like valgrind.

It makes sense! Thanks for the explanation.

amomchilov · 2025-01-08T19:28:48Z

@soutaro

Let me confirm that: all other usage of Ruby symbols will finally be replaced to rbs_constant_pool to make the implementation independent to MRI C API, right?

Eventually... yes! But that requires we introduce the C data structures (to replace the Ruby objects) and translation layer (to translate to Ruby objects, for the Ruby API), which aren't ready to ship yet.

This PR chips away by migrating all usages related to location child names and typevars, and leaves the rest as-is for now.

soutaro

Thanks! 🎉

amomchilov changed the title ~~Extract out usages of Ruby Symbols and IDs~~ Extract out usages of Ruby IDs Dec 18, 2024

amomchilov mentioned this pull request Dec 18, 2024

[Epic] Pure C API #2133

Open

7 tasks

amomchilov force-pushed the ruby-based-constant-pool branch 2 times, most recently from 4ee0431 to fa6470f Compare December 18, 2024 05:16

amomchilov commented Dec 18, 2024

View reviewed changes

amomchilov force-pushed the ruby-based-constant-pool branch 2 times, most recently from 0b4f69e to 3ae33c6 Compare December 18, 2024 06:12

amomchilov marked this pull request as ready for review December 18, 2024 06:16

amomchilov force-pushed the ruby-based-constant-pool branch 2 times, most recently from 7fdab83 to 6464270 Compare January 6, 2025 18:31

amomchilov commented Jan 6, 2025

View reviewed changes

Extract rbs_constant_pool_t interface

138e4ec

amomchilov force-pushed the ruby-based-constant-pool branch from 6464270 to 4153580 Compare January 6, 2025 18:36

amomchilov added 2 commits January 6, 2025 13:39

Use rbs_constant_pool for typevar table

31ffaa0

Use rb_ary_entry to skip INT2FIX

94b0308

amomchilov force-pushed the ruby-based-constant-pool branch from 4153580 to 94b0308 Compare January 6, 2025 18:39

amomchilov mentioned this pull request Jan 6, 2025

Reimplement the RBS Constant Pool in pure C #2221

Merged

soutaro reviewed Jan 8, 2025

View reviewed changes

soutaro approved these changes Jan 9, 2025

View reviewed changes

soutaro added this pull request to the merge queue Jan 9, 2025

Merged via the queue into ruby:master with commit 991bacb Jan 9, 2025
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract out usages of Ruby IDs #2175

Extract out usages of Ruby IDs #2175

amomchilov commented Dec 18, 2024 •

edited

Loading

amomchilov Dec 18, 2024

amomchilov Dec 18, 2024

soutaro Jan 8, 2025

amomchilov Jan 9, 2025

amomchilov Dec 18, 2024

amomchilov Jan 6, 2025

soutaro commented Jan 8, 2025

soutaro Jan 8, 2025

ParadoxV5 Jan 8, 2025

amomchilov Jan 8, 2025

soutaro Jan 9, 2025

amomchilov commented Jan 8, 2025 •

edited

Loading

soutaro left a comment

Extract out usages of Ruby IDs #2175

Extract out usages of Ruby IDs #2175

Conversation

amomchilov commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soutaro commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov commented Jan 8, 2025 • edited Loading

soutaro left a comment

Choose a reason for hiding this comment

amomchilov commented Dec 18, 2024 •

edited

Loading

amomchilov commented Jan 8, 2025 •

edited

Loading