Project

General

Profile

Bug #18007

Updated by Eregon (Benoit Daloze) over 2 years ago

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. 

 https://github.com/ruby/ruby/blob/6963f8f743b42f9004a0879cd66c550f18987352/doc/extension.rdoc#label-Write+the+C+Code says: 
 > Since Object.allocate allocates an ordinary T_OBJECT type (instead of T_DATA), it's important to either use rb_define_alloc_func() to overwrite it or rb_undef_alloc_func() to delete it. 

 (note: which matters when using TypedData_Make_Struct/TypedData_Wrap_Struct as the native pointer is supplied without calling the class alloc function). 

 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.

Back