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

[ruby-core:113632] [Ruby master Bug#19691] Case insensitive file systems, require filename casing
by MSP-Greg (Greg L) 29 May '23

29 May '23

Issue #19691 has been reported by MSP-Greg (Greg L). ---------------------------------------- Bug #19691: Case insensitive file systems, require filename casing https://bugs.ruby-lang.org/issues/19691 * Author: MSP-Greg (Greg L) * Status: Open * Priority: Normal * Backport: 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN ---------------------------------------- I don't know what I think of this (expected or bug), but recently in a forum I saw an issue that I don't recall accidentally doing myself. An equivalent example: ``` ruby -rsingleton -rSingleton -e "puts 'case issue'" ``` Note the change of casing with `Singleton`. On case insensitive file systems, the above will double load the file, which errors. Obviously, on case sensitive file systems, it errors with `cannot load such file -- Singleton` So, options are: A. No change to current behavior, which I'm fine with. B. If the OS is case insensitive, don't reload the file, and maybe a warning that `Singleton` and `singleton` are equivalent. -- https://bugs.ruby-lang.org/

3 6

[ruby-core:113689] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by ianks (Ian Ker-Seymer) 28 May '23

28 May '23

Issue #19057 has been updated by ianks (Ian Ker-Seymer). > With one tweak: not int rb_ioptr_descriptor(struct rb_io *ioptr); but int rb_io_descriptor(VALUE io);. > The goal is to not expose the struct in the C API, so let's not expose it (even if it becomes just an opaque pointer). I have mixed feelings about this. On one hand, it does successfully hide the implementation. On the other, casting pointers to unsigned ints has implications for pointer provenance models. Would be nice to get the opaqueness without the int-cast. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103334 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113682] [Ruby master Misc#17591] Test frameworks and REPLs do not show deprecation warnings by default
by Eregon (Benoit Daloze) 27 May '23

27 May '23

Issue #17591 has been updated by Eregon (Benoit Daloze). IMO we should find a way for RSpec to enable them by default, it is hurting RSpec users and destroying ruby-core's efforts to provide nice migration paths. Maybe more voices on that RSpec issue could help to convince the RSpec devs (but probably not). Or potentially workaround by enabling them e.g. when running tests of a rails app. RSpec does have `-w` which enables them, so another potential way could be to teach rspec users about that, maybe via a gem-install-time message? RSpec users will likely learn about it when something breaks without warning, but then they will probably think it's CRuby's fault when it's RSpec's fault. I agree enabling deprecation warnings by default seems best, and then people who don't want to see it can just set `Warning[:deprecated] = false`. Let's see what others think, but I suspect many would be reticent to change this again. ---------------------------------------- Misc #17591: Test frameworks and REPLs do not show deprecation warnings by default https://bugs.ruby-lang.org/issues/17591#change-103326 * Author: Eregon (Benoit Daloze) * Status: Closed * Priority: Normal ---------------------------------------- Various people in #16345 said that: > The issue can be mitigated if all test frameworks enable all deprecation warnings. > The developer's practice can be supported by tools, such as test frameworks enable deprecation warnings automatically. And this was used as a base to disable deprecation warnings by default in Ruby 2.7.2. However, it seems no test frameworks or REPLs actually show deprecation warnings by default! So Ruby developers will typically never see deprecation warnings for keyword arguments, and it will just break when they try Ruby 3.0. I think only MSpec does `Warning[:deprecated] = true`, whether or not `-w` is passed, which seems the right thing to do. Currently, RSpec enables `Warning[:deprecated] = true` only for `rspec -w`. Same for `test/unit` 3.3.4 shipped with 2.7.2. IRB in 2.7.2 does not show deprecated warnings. Same for `pry`. I think ruby-core needs to have a clear message here, like: > All test frameworks and REPLs should include this snippet and run it before running tests: `Warning[:deprecated] = true if Warning.respond_to?(:[]=)`. > This is important so that developers see warnings in development, and that they see the warnings before updating to the next Ruby version. > Developers can choose to disable deprecation warnings explicitly if they want with `Warning[:deprecated] = false`. And I think it would be good that ruby-core makes a PR or an issue to the main test frameworks/REPLs to show examples. P.S.: if someone wants to disable all warnings with `-W0` or `$VERBOSE = nil`, it will indeed disable them all, including deprecation warnings, so there is no need to check `$VERBOSE` for setting `Warning[:deprecated] = true`. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113681] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by ioquatix (Samuel Williams) 27 May '23

27 May '23

Issue #19057 has been updated by ioquatix (Samuel Williams). Given the discussion here, I just want to aim for "Hide all the details". Thanks everyone, the discussion has been most helpful. I especially like the discussions around undefined behaviour. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103325 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113680] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by Eregon (Benoit Daloze) 27 May '23

27 May '23

Issue #19057 has been updated by Eregon (Benoit Daloze). ioquatix (Samuel Williams) wrote in #note-3: > Maybe it's CRuby problem, but there is a performance issue with such an implementation which has to extract and follow the pointer to the struct over and over again. It's pretty rare to access multiple fields of `rb_io_t` in code I have seen, so then performance-wise it is the same if accessing a single field. When accessing two fields it would be indeed an extra indirection, although I highly doubt this would be measurable overhead. Coming from the other way, on all Ruby implementations besides CRuby, exposing `struct rb_io *ioptr` would mean allocating a struct just to support those accesses, for every IO instance, possibly lazily. That's a much much bigger overhead than an extra pointer indirection. As HPy shows, a fast and portable C API does not expose structs. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103324 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113679] [Ruby master Misc#17591] Test frameworks and REPLs do not show deprecation warnings by default
by byroot (Jean Boussier) 27 May '23

27 May '23

Issue #17591 has been updated by byroot (Jean Boussier). Given that RSpec doesn't want to enable deprecation warnings by default, and that they were turned off mostly because the keyword related deprecations were very verbose, should we reconsider enabling them by default? I kinda feel it's pointless going through deprecation cycles if a large part of the community is never warned that their code will break in the future... ---------------------------------------- Misc #17591: Test frameworks and REPLs do not show deprecation warnings by default https://bugs.ruby-lang.org/issues/17591#change-103323 * Author: Eregon (Benoit Daloze) * Status: Closed * Priority: Normal ---------------------------------------- Various people in #16345 said that: > The issue can be mitigated if all test frameworks enable all deprecation warnings. > The developer's practice can be supported by tools, such as test frameworks enable deprecation warnings automatically. And this was used as a base to disable deprecation warnings by default in Ruby 2.7.2. However, it seems no test frameworks or REPLs actually show deprecation warnings by default! So Ruby developers will typically never see deprecation warnings for keyword arguments, and it will just break when they try Ruby 3.0. I think only MSpec does `Warning[:deprecated] = true`, whether or not `-w` is passed, which seems the right thing to do. Currently, RSpec enables `Warning[:deprecated] = true` only for `rspec -w`. Same for `test/unit` 3.3.4 shipped with 2.7.2. IRB in 2.7.2 does not show deprecated warnings. Same for `pry`. I think ruby-core needs to have a clear message here, like: > All test frameworks and REPLs should include this snippet and run it before running tests: `Warning[:deprecated] = true if Warning.respond_to?(:[]=)`. > This is important so that developers see warnings in development, and that they see the warnings before updating to the next Ruby version. > Developers can choose to disable deprecation warnings explicitly if they want with `Warning[:deprecated] = false`. And I think it would be good that ruby-core makes a PR or an issue to the main test frameworks/REPLs to show examples. P.S.: if someone wants to disable all warnings with `-W0` or `$VERBOSE = nil`, it will indeed disable them all, including deprecation warnings, so there is no need to check `$VERBOSE` for setting `Warning[:deprecated] = true`. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113678] [Ruby master Feature#19057] Hide implementation of `rb_io_t`.
by kjtsanaktsidis (KJ Tsanaktsidis) 27 May '23

27 May '23

Issue #19057 has been updated by kjtsanaktsidis (KJ Tsanaktsidis). I did a bit of research on this topic this evening. Firstly, some technical notes r.e. undefined behaviour. My understanding is that "Current proposal" is really undefined behaviour. This is in the C standard, section 6.5.2.3, point 6 (http://www.open-std.org/jtc1/sc22/wg14/www/abq/c17_updated_proposed_fdis.pdf) > One special guarantee is made in order to simplify the use of unions: if a union contains several > structures that share a common initial sequence (see below), and if the union object currently contains > one of these structures, it is permitted to inspect the common initial part of any of them anywhere > that a declaration of the completed type of the union is visible. Two structures share a common initial > sequence if corresponding members have compatible types (and, for bit-fields, the same widths) for a > sequence of one or more initial members It seems that this "common initial sequence" rule is really only for structures accessed through unions. Another fun gotcha is that, even if you _do_ do all the accesses through a union, the compiler can assume that `rb_io_public_t *` and `rb_io_private_t *` should never alias each other (which is the exact opposite of what you want). E.g. - this program has a different result depending on whether optimizations are on or not - http://coliru.stacked-crooked.com/a/b57c8dd9e2ef3a02 I think to be technically correct, we would need to go for the "Nested public interface" approach - structs _can_ be converted to a pointer to their first field according to the C standard (although I guess the _reverse_ cast from `rb_io_public_t *` back to `rb_io_private_t *` would be UB?) ------------ I don't think this matters though, because I agree that, looking at the contents of `rb_io_t`, almost none of this should be public API, and we should strive to encapsulate it fully, like "Hide all details" (or like @Eregon suggested too). I did a Github code search for usages of `rb_io_t`, and _all_ of the usages of it I could find basically fell into this pattern. ``` VALUE some_io = /* from somewhere */; rb_io_t *fptr; GetOpenFile(some_io, fptr) // call an io.h method that takes rb_io_t * rb_io_set_nonblock(fptr); // or read the fd member and do something with that do_something_external_with_fd(fptr->fd); ``` I think we could avoid 99% of the breakage, and still hide all the implementation details of rb_io_t, by basically redefining `rb_io_t` to contain just the FD, and define a new internal, opaque type for internal IO use. We would do this like so: Firstly, `struct RFile` is currently in include/ruby/internal/core/rfile.h as: ``` struct rb_io_t; struct RFile { struct RBasic basic; struct rb_io_t *fptr; }; ``` There are two spare words in there, and we would use one to add a pointer to a new structure. The definition of that structure would _not_ be provided in public headers anyhere. So, we would change RFile in include/ruby/internal/core/rfile.h to be: ``` struct rb_io_t; struct rb_io_impl; struct RFile { struct RBasic basic; struct rb_io_t *fptr; struct rb_io_impl *impl; }; ``` Then, we would change the definition of `struct rb_io_t` in include/ruby/io.h to be: ``` typedef struct rb_io_t { VALUE self; int fd; // _everything_ else is removed } rb_io_t; ``` We would move all of its current contents to a new struct in internal/io.h: ``` struct rb_io_impl { // All the juicy implementation details go here - _except_ no need to duplicate `self` and `fd`. }; ``` The current definition of `GetOpenFile` essentially does `RFILE(obj)->fptr`, so that would return the newly-slimmed-down public `rb_io_t`. Attempts to read the file descriptor off that would continue to work. Other methods in include/ruby/io.h which take a `rb_io_t *` as an argument, like e.g. `rb_io_set_nonblock`, would do `RFILE(fptr->self)->impl` to get access to the implementation details struct. We could also define variants of these which took the `VALUE` instead of the `rb_io_t *` to avoid the pointer-chase of `fptr->self` if desired. The downside of this approach is that, as you identified, it costs an extra indirection in the implementation of most IO methods to obtain the impl from the VALUE. But, the benefits are that we get almost total encapsulation whilst maintaining backwards compatibility with almost all existing code. We can also shuffle fields between the private & public structs depending on observed compatibility issues found in the wild too. ---------------------------------------- Feature #19057: Hide implementation of `rb_io_t`. https://bugs.ruby-lang.org/issues/19057#change-103322 * Author: ioquatix (Samuel Williams) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- In order to make improvements to the IO implementation like <https://bugs.ruby-lang.org/issues/18455>, we need to add new fields to `struct rb_io_t`. By the way, ending types in `_t` is not recommended by POSIX, so I'm also trying to rename the internal implementation to drop `_t` where possible during this conversion. Anyway, we should try to hide the implementation of `struct rb_io`. Ideally, we don't expose any of it, but the problem is backwards compatibility. So, in order to remain backwards compatibility, we should expose some fields of `struct rb_io`, the most commonly used one is `fd` and `mode`, but several others are commonly used. There are many fields which should not be exposed because they are implementation details. ## Current proposal The current proposed change <https://github.com/ruby/ruby/pull/6511> creates two structs: ```c // include/ruby/io.h #ifndef RB_IO_T struct rb_io { int fd; // ... public fields ... }; #else struct rb_io; #endif // internal/io.h #define RB_IO_T struct rb_io { int fd; // ... public fields ... // ... private fields ... }; ``` However, we are not 100% confident this is safe according to the C specification. My experience is not sufficiently wide to say this is safe in practice, but it does look okay to both myself, and @Eregon + @tenderlovemaking have both given some kind of approval. That being said, maybe it's not safe. There are two alternatives: ## Hide all details We can make public `struct rb_io` completely invisible. ```c // include/ruby/io.h #define RB_IO_HIDDEN struct rb_io; int rb_ioptr_descriptor(struct rb_io *ioptr); // accessor for previously visible state. // internal/io.h struct rb_io { // ... all fields ... }; ``` This would only be forwards compatible, and code would need to feature detect like this: ```c #ifdef RB_IO_HIDDEN #define RB_IOPTR_DESCRIPTOR rb_ioptr_descriptor #else #define RB_IOPTR_DESCRIPTOR(ioptr) rb_ioptr_descriptor(ioptr) #endif ``` ## Nested public interface Alternatively, we can nest the public fields into the private struct: ```c // include/ruby/io.h struct rb_io_public { int fd; // ... public fields ... }; // internal/io.h #define RB_IO_T struct rb_io { struct rb_io_public public; // ... private fields ... }; ``` ## Considerations I personally think the "Hide all details" implementation is the best, but it's also the lest compatible. This is also what we are ultimately aiming for, whether we decide to take an intermediate "compatibility step" is up to us. I think "Nested public interface" is messy and introduces more complexity, but it might be slightly better defined than the "Current proposal" which might create undefined behaviour. That being said, all the tests are passing. -- https://bugs.ruby-lang.org/

1 0

[ruby-core:113498] [Ruby master Feature#19644] Module::current to complement Module::nesting
by bughit (bug hit) 25 May '23

25 May '23

Issue #19644 has been reported by bughit (bug hit). ---------------------------------------- Feature #19644: Module::current to complement Module::nesting https://bugs.ruby-lang.org/issues/19644 * Author: bughit (bug hit) * Status: Open * Priority: Normal ---------------------------------------- Module::current == Module::nesting[0] but without needlessly walking the entire nesting hierarchy. Could be useful for debugging/logging. It could also be a Kernel global (like __method__) or a keyword (like __FILE__) -- https://bugs.ruby-lang.org/

4 11

[ruby-core:113538] [Ruby master Feature#19682] ability to get a reference to the "default definee"
by bughit (bug hit) 25 May '23

25 May '23

Issue #19682 has been reported by bughit (bug hit). ---------------------------------------- Feature #19682: ability to get a reference to the "default definee" https://bugs.ruby-lang.org/issues/19682 * Author: bughit (bug hit) * Status: Open * Priority: Normal ---------------------------------------- https://blog.yugui.jp/entry/846 "default definee" is a pretty important context so why not make it easy to identify? Could be a Module class method or a global method (Kernel) or a keyword. -- https://bugs.ruby-lang.org/

6 27

[ruby-core:113652] [Ruby master Misc#19122] Use MADV_DONTNEED instead of MADV_FREE when freeing a Fiber's stack
by ioquatix (Samuel Williams) 25 May '23

25 May '23

Issue #19122 has been updated by ioquatix (Samuel Williams). If you want to use a specific mode (OS specific), you can do this: On Linux, find the mode, e.g. `MADV_DONTNEED = 6` (https://github.com/torvalds/linux/blob/933174ae28ba72ab8de5b35cb7c98fc21123…) Shift it one bit to the left (i.e. multiply by 2) `= 12`. Then run Ruby like this: ``` > RUBY_SHARED_FIBER_POOL_FREE_STACKS=12 ruby ``` This will force the use of `MADV_DONTNEED`. However, no validation is done, so the OS may not accept it. So it will also emit a warning that this behaviour is OS dependent and may crash your Ruby interpreter. With this in place, you should be able to test your code. As for making this the default, I suppose we could consider it but we'd need to confirm the performance impact. ---------------------------------------- Misc #19122: Use MADV_DONTNEED instead of MADV_FREE when freeing a Fiber's stack https://bugs.ruby-lang.org/issues/19122#change-103295 * Author: smcgivern (Sean McGivern) * Status: Open * Priority: Normal * Assignee: ioquatix (Samuel Williams) ---------------------------------------- I'd like to propose that Ruby stops using MADV_FREE when freeing a Fiber's stack, and switches to using MADV_DONTNEED even when MADV_FREE is supported. MADV_FREE is used in one place in the Ruby codebase, when freeing the stack of a freed Fiber: https://git.ruby-lang.org/ruby.git/tree/cont.c#n683 The comment for `fiber_pool_stack_free` says: ```c // We advise the operating system that the stack memory pages are no longer being used. // This introduce some performance overhead but allows system to relaim memory when there is pressure. ``` Where possible (i.e. on Linux 4.5 and later), `fiber_pool_stack_free` uses `MADV_FREE` over `MADV_DONTNEED`. This has the side effect that memory statistics such as RSS will not reduce until and unless the OS actually reclaims that memory. If that doesn't happen, then the reported memory usage via RSS will be much higher than the 'real' memory usage. If this was pervasive throughtout the Ruby codebase then that would be one thing, but currently this is just for Fiber. This means that: 1. A program that doesn't use Fiber will have somewhat reliable RSS statistics on recent Linux. 2. A program that heavily uses Fiber (such as something using Async::HTTP) will see an inflated RSS statistic. Go made a similar change to the one I'm proposing here for similar reasons: https://github.com/golang/go/issues/42330 > While `MADV_FREE` is somewhat faster than `MADV_DONTNEED`, it doesn't affect many of the statistics that `MADV_DONTNEED` does until the memory is actually reclaimed. This generally leads to poor user experience, like confusing stats in `top` and other monitoring tools; and bad integration with management systems that respond to memory usage. > [...] > I propose we change the default to prefer `MADV_DONTNEED` over `MADV_FREE`, to favor user-friendliness and minimal surprise over performance. I think it's become clear that Linux's implementation of `MADV_FREE` ultimately doesn't meet our needs. As an aside, MADV_FREE was not used in Ruby 3.1 (https://bugs.ruby-lang.org/issues/19101) and I haven't found any bugs filed about this behaviour other than that one. -- https://bugs.ruby-lang.org/

1 0