[ruby-core:104952] [Ruby master Feature#18007] Help developers of C extensions meet requirements in "doc/extension.rdoc"
From:
"nobu (Nobuyoshi Nakada)" <noreply@...>
Date:
2021-08-17 09:29:31 UTC
List:
ruby-core #104952
Issue #18007 has been updated by nobu (Nobuyoshi Nakada). Eregon (Benoit Daloze) wrote in #note-7: > I'm confused, why does `Class#allocate` not simply use the alloc function set with `rb_define_alloc_func`? You're confused by `Class#allocate` which calls the allocator set for each `T_DATA` and `rb_class_allocate` which implements the default allocator for `T_OBJECT`. The former method now calls the latter function set with `rb_define_alloc_func`. ---------------------------------------- Feature #18007: Help developers of C extensions meet requirements in "doc/extension.rdoc" https://bugs.ruby-lang.org/issues/18007#change-93316 * Author: mdalessio (Mike Dalessio) * Status: Open * Priority: Normal ---------------------------------------- A pull request for this feature has been submitted at https://github.com/ruby/ruby/pull/4604 ## Problem being solved This option is intended to help developers of C extensions to check if their code meets the requirements explained in "doc/extension.rdoc". Specifically, I want to ensure that `T_DATA` object classes undefine or redefine the `allocate` method. There is currently no easy way for an author of a C extension to easily see where they have made the mistake of letting the default `allocate` class method remain. ## Description of the solution Compiled with this option, Ruby will warn when a `T_DATA` object is created whose class has not undefined or redefined the `allocate` method. A new function is defined, `rb_data_object_check`. That function is called from `rb_data_object_wrap()` and `rb_data_typed_object_wrap()` (which implement the `Data_Wrap_Struct` family of macros). The warning, when emitted, looks like this: ``` warning: T_DATA class Nokogiri::XML::Document should undefine or redefine the allocate method, please see doc/extension.rdoc ``` ## Examples of this problem in the wild Using this option, I found that [many of Nokogiri's classes needed to undefine `allocate`](https://github.com/sparklemotion/nokogiri/commit/c5ba3a5). This PR also updates these core Ruby classes by undefining `allocate`: - `ObjectSpace::InternalObjectWrapper` - `Socket::Ifaddr` ## Questions for reviewers __Does this check really need to be behind a configuration option?__ Performance impact is very small (see benchmarks below), but I put it behind a flag because I am worried that there may be a many C extensions that would emit warnings at runtime, and the users of those extensions cannot fix the problem and so would mostly just be annoyed. __Should this warning be emitted with the `deprecated` category?__ ## Benchmarking I benchmarked this code by allocating `Nokogiri::XML::NodeSet`s in a loop. This is a class with a [relatively simple `allocate` function](https://github.com/sparklemotion/nokogiri/blob/6d688d8c0f3351797e9576d3710acf458582bb30/ext/nokogiri/xml_node_set.c#L441-L464). The runs cover the four combinations of enabled/disabled, and warnings/no-warnings. ``` ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux] Warming up -------------------------------------- disabled, warn=false 490.143k i/100ms Calculating ------------------------------------- disabled, warn=false 4.863M (1.5%) i/s - 49.014M in 10.081177s ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux] Warming up -------------------------------------- disabled, warn=true 483.070k i/100ms Calculating ------------------------------------- disabled, warn=true 4.839M (ア 1.4%) i/s - 48.790M in 10.083899s Comparison: disabled, warn=false: 4863064.0 i/s disabled, warn=true: 4839310.1 i/s - same-ish: difference falls within error ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux] Warming up -------------------------------------- enabled, warn=false 484.398k i/100ms Calculating ------------------------------------- enabled, warn=false 4.840M (ア 1.9%) i/s - 48.440M in 10.011854s Comparison: disabled, warn=false: 4863064.0 i/s enabled, warn=false: 4840123.2 i/s - same-ish: difference falls within error disabled, warn=true: 4839310.1 i/s - same-ish: difference falls within error ruby 3.1.0dev (2021-06-25T04:02:18Z flavorjones-extens.. de943189aa) [x86_64-linux] Warming up -------------------------------------- enabled, warn=true 492.200k i/100ms Calculating ------------------------------------- enabled, warn=true 4.866M (ア 2.1%) i/s - 48.728M in 10.017455s Comparison: enabled, warn=true: 4866434.8 i/s disabled, warn=false: 4863064.0 i/s - same-ish: difference falls within error enabled, warn=false: 4840123.2 i/s - same-ish: difference falls within error disabled, warn=true: 4839310.1 i/s - same-ish: difference falls within error ``` My conclusion is that the performance impact is very small, and we could omit the option if the Ruby core maintainers decide this behavior should be on by default. -- https://bugs.ruby-lang.org/ Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=unsubscribe> <http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>