Project

General

Profile

Actions

Feature #13000

closed

Implement Set#include? with Hash#include?

Added by headius (Charles Nutter) over 7 years ago. Updated over 3 years ago.

Status:
Feedback
Target version:
-
[ruby-core:78467]
Tags:

Description

Why does Set#include? not call Hash#include?? Currently it calls Hash#[].

The protocol of Set already use Hash#include? for ==.

diff --git a/lib/set.rb b/lib/set.rb
index 43c388c..f3dbe2d 100644
--- a/lib/set.rb
+++ b/lib/set.rb
@@ -230,7 +230,7 @@ def flatten!
   #
   # See also Enumerable#include?
   def include?(o)
-    @hash[o]
+    @hash.include?(o)
   end
   alias member? include?
 

Updated by shyouhei (Shyouhei Urabe) about 7 years ago

  • Status changed from Open to Assigned
  • Assignee set to knu (Akinori MUSHA)

Updated by knu (Akinori MUSHA) about 7 years ago

It originally used Hash#include?, but changed to use Hash#[] to benefit from the optimized dispatch VM instruction for [] (opt_aref). ([Misc #10754])

Running a benchmark, I can observe that Hash#[] actually has an advantage over include? in performance (up to ~1.2x faster) but the "optimization" may only apply to CRuby. Do you think we should have a straightforward implementation for a library shared between Ruby implementations, or is it OK to leave this if I add a comment to explain why?

Actions #3

Updated by knu (Akinori MUSHA) over 6 years ago

  • Status changed from Assigned to Feedback

Updated by headius (Charles Nutter) over 6 years ago

I would prefer the straightforward implementation, but I have some bias.

In JRuby, the [] method generally is more expensive, because it might be String#[] with a Regex, which needs to be able to set $~, so we deoptimize some things when [] is being called.

What's good for MRI here is bad for JRuby :-)

I guess the real question here is whether it would matter if JRuby just used Hash#include? in our version of the library. I think there might be some oddities around nil, but that already seems pretty odd in a Set.

Actions #5

Updated by jeremyevans0 (Jeremy Evans) over 3 years ago

  • Tracker changed from Bug to Feature
  • Backport deleted (2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN)
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0