Avoid use of id2ref for weak mapping

[headius]

Revisiting my report in https://bugs.ruby-lang.org/issues/15711 and trying to finally get rid of the use of ObjectSpace._id2ref in drb. That report was closed but never actually fixed in DRb.

The implementation of ObjectSpace._id2ref on JRuby (and TruffleRuby) is very expensive, and on JRuby we normally do not have it enabled because it impacts performance of the entire runtime.

This alternate implementation uses the weakref library to build a weak ID map. It could possibly use the new ObjectSpace::WeakMap, but until recently that did not support Fixnum keys and I did not want to break DRb for older Ruby versions.

Unfortunately, due to the lack of "reference queue" support in Ruby's Weakref, this implementation must do a full scan for dead references. This may be possible to improve, and would probably be resolved by using WeakMap.

We recently got another report pry-remote not working on JRuby. pry-remote uses DRb, and DRb still uses ObjectSpace._id2ref to simulate a weak map of objects. We would like to get this fixed and released so users can use pry-remote without enabling full ObjectSpace support.

Notes:

• This is only enabled for JRuby in this patch. It could be enabled for all Rubies.
• When using this implementation on CRuby, all tests pass.
• No attempt is made to ensure the thread-safety of the weak Hash. A mutex could be introduced for this, or it may be ok if we switch to WeakMap.

Edit: Modified to use existing thread-safe WeakMap-based implementation WeakIdConv by default. Without it being default, users of DRb will always end up using the _id2ref version. We should simply eliminate that possibility.

[eregon]

It could possibly use the new ObjectSpace::WeakMap, but until recently that did not support Fixnum keys and I did not want to break DRb for older Ruby versions.

In what version was that fixed?
This gem is 2.7+:

drb/drb.gemspec

Line 16 in 10fceea

spec.required_ruby_version = Gem::Requirement.new(">= 2.7.0")

And 2.7 seems to support ObjectSpace::WeakMap fixnum keys:

irb(main):005:0> RUBY_DESCRIPTION
=> "ruby 2.7.7p221 (2022-11-24 revision 168ec2b1e5) [x86_64-linux]"
irb(main):006:0> m=ObjectSpace::WeakMap.new
=> #<ObjectSpace::WeakMap:0x00000000028174f8>
irb(main):007:0> m[2] = 3
=> 3
irb(main):008:0> m
=> #<ObjectSpace::WeakMap:0x00000000028174f8: 2 => 3>

[headius]

A version with WeakMap also passes tests:

      def initialize
        @id2ref = ObjectSpace::WeakMap.new
      end

      # Convert an object reference id to an object.
      #
      # This implementation looks up the reference id in the local object
      # space and returns the object it refers to.
      def to_obj(ref)
        @id2ref[ref]
      end

      # Convert an object into a reference id.
      #
      # This implementation returns the object's __id__ in the local
      # object space.
      def to_id(obj)
        return if obj == nil

        id = obj.__id__
        @id2ref[id] = obj
        id
      end

And 2.7 seems to support ObjectSpace::WeakMap fixnum keys:

If this gem is only supported on 2.7 or higher, then the WeakMap version would be fine.

[headius]

It appears that WeakMap was updated to allow Integer keys in https://bugs.ruby-lang.org/issues/16035 (for 2.7).

The limitation mentioned there on JRuby (Fixnum idempotency) was resolved by having a separate value-based weak map for such objects.

[eregon]

Big 👍 to remove the usage of ObjectSpace._id2ref.
I think it would be best to use WeakMap unconditionally, because it is cleaner and makes it possible to eventually deprecate _id2ref.

From https://bugs.ruby-lang.org/issues/15711#note-12 I found there is already a WeakMap implementation in
https://github.com/ruby/drb/blob/master/lib/drb/weakidconv.rb

Could we change DRb so it uses that one by default and remove the _id2ref one?

[headius]

I found there is already a WeakMap implementation

Ah I did not notice that had gotten merged into the gem. It was merged as part of my original issue years ago, but never used.

I will adjust the patch to unconditionally use the WeakIdConv implementation.

[headius]

Digging deeper it appears that WeakIdConv was added with a mechanism for selecting it, but since it is not set as default we still see DrbIdConv being used everywhere.

I'm making modifications to deprecate the old version, use WeakIdConv by default, and possibly move the old version to a separate file.

[headius]

I'm making modifications to deprecate the old version, use WeakIdConv by default, and possibly move the old version to a separate file.

This is done. It could be cleaner. We could also just remove the _id2ref version altogether, replace it with the WeakMap version, and eliminate the weakidconv.rb file.

[eregon]

LGTM

[casperisfine]

I filed a PR to deprecate ObjectSpace._id2ref, so it would be valuable to not use it even on MRI: ruby/ruby#13157

[byroot]

This LGTM and was mentioned and approved years ago in https://bugs.ruby-lang.org/issues/15408, so 👍

@hsbt you seem to be the maintainer of drb ? Any objection to me merging?

[kou]

I'm talking about this with @seki, the author of DRb.
Please wait for a while...

[kou]

#35

It's based on https://gist.github.com/seki/773ab9ffe403a5197e82e1a90cb2c583 that is implemented by @seki.