Skip to content

Use a weak equality map instead of _id2ref #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
headius wants to merge 1 commit into from
Closed

Use a weak equality map instead of _id2ref #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 53 additions & 12 deletions lib/drb/drb.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
require 'socket'
require 'io/wait'
require 'monitor'
require 'weakref'
require_relative 'eq'

#
Expand Down Expand Up @@ -357,27 +358,67 @@ class DRbConnError < DRbError; end
# For alternative mechanisms, see DRb::TimerIdConv in drb/timeridconv.rb
# and DRbNameIdConv in sample/name.rb in the full drb distribution.
class DRbIdConv
MUTEX = Mutex.new

def initialize
@id2ref = {}
Copy link
Member

@eregon eregon Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would ObjectSpace::WeakMap make sense here? That would keep the entries as long as the value is alive, which seems the desired semantic here.

Copy link
Author

@headius headius Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works only for Ruby 2.7 and higher, since it would require immediates (integer IDs) as keys. That restriction might be ok, since 2.6 did not ship drb as a gem. It would mean this gem is not usable at all on 2.6.

$ rvm ruby-2.6.7 do ruby -e 'ObjectSpace::WeakMap.new[self.__id__] = self'
-e:1:in `[]=': cannot define finalizer for Integer (ArgumentError)
	from -e:1:in `<main>'

Copy link
Member

@eregon eregon Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gem specifies >= 2.7 so that seems fine:

drb/drb.gemspec

Line 16 in 82643ca

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

Also, WeakIdConv already uses ObjectSpace::WeakMap.
I think making WeakIdConv the default is probably the safest way.

Copy link
Author

@headius headius Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that we should also remove the id2ref version in order to start moving away from having any official code that uses it. The WeakMap version will be more reliable and should work on 2.7+ compatible implementations.

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.
# and returns the object it refers to.
def to_obj(ref)
ObjectSpace._id2ref(ref)
_get(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)
case obj
when Object
obj.nil? ? nil : obj.__id__
when BasicObject
obj.__id__
obj.nil? ? nil : _put(obj)
Copy link
Contributor

@jeremyevans jeremyevans Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong. BasicObject#nil? is not defined. You should probably keep the case obj, and just switch the obj.__id__ to _put(obj) in both when branches. I would guess that is what causes the test failure for basic objects.

end

private def _safe_lock
Thread.handle_interrupt(Exception => :never) do
MUTEX.lock
begin
yield
ensure
MUTEX.unlock
Copy link
Contributor

@jeremyevans jeremyevans Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idiomatic way here would be to use MUTEX.synchronize(&block). However, maybe there a reason the Thread.handle_interrupt and explicit lock/unlock was done instead. If so, could you add a comment why?

Copy link
Author

@headius headius Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly to avoid the overhead of calling the block in synchronize, since this will be hit hard by anyone using a remote object. Is that worth a comment?

Copy link
Contributor

@jeremyevans jeremyevans Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would switch to MUTEX.synchronize(&block) unless you have a benchmark showing the performance differences are significant enough to warrant this more complex code. If the performance differences are significant, then keeping the code and adding a comment explaining it seems best. My uneducated guess would be that the overhead from having a remote object is much higher than the overhead of a mutex synchronization, even on localhost.

Copy link
Author

@headius headius Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. The block form should be slower on CRuby, but it may be noise compared to the weak map and other remote object overhead.

end
end
end

private def _put(obj)
id = obj.__id__
_safe_lock do
_clean_locked
@id2ref[id] = WeakRef.new(obj)
end
id
end

private def _get(id)
_safe_lock do
weakref = @id2ref[id]
if weakref
result = weakref.__getobj__ rescue nil
Copy link
Contributor

@jeremyevans jeremyevans Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this handles false values correctly, returning them as nil. I would think you would only want to delete from the hash if __getobj__ raises, and otherwise return the result. Explicit nil values are probably OK, since I'm guessing they result in the same behavior.

if result
return result
else
@id2ref.delete id
end
end
end
nil
end

private def _clean
_safe_lock { _clean_locked }
end

private def _clean_locked
dead = []
@id2ref.each { |id, weakref| dead << id unless weakref.weakref_alive? }
dead.each { |id| @id2ref.delete(id) }
end
end

# Mixin module making an object undumpable or unmarshallable.
Expand Down