-
-
Notifications
You must be signed in to change notification settings - Fork 925
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.
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
improve Ruby's Set performance #4334
Comments
@kares what do you plan on changing? Ruby's Set is implemented in Ruby itself and is backed by a Ruby Hash. Or are you thinking of a full Java native version? |
@enebo hey! no plan so far. thought I report - to have some feedback whether its preferable the way it is. |
@kares maintenance would be a primary reason but if set is significant for something as important as Rails perhaps we should be looking at things that way? |
looked into whether it gets even slower with Rails (due AS patching) ... seems that
.. with active_support/core_ext loaded - guess it's not worth the effort unless real-world use-cases shows up. |
set.rb definitely could use a rework. I've thought about writing it in Java a few times to get MAXIMUM SPEED but maintaining the "hash wrapper" behavior is tricky. I'm actually rather impressed we only lose this much for the set.rb impl, since every operation is wrapped by Ruby code. |
Oh other rework item needed: make Set thread-safe. |
@headius yy, also kind of expected it to be slower (maybe other ops are - only micro-benched |
One item slowing this (and other) Set methods down is the use of before/after:
I'll open a bug with MRI to change that include? impl. |
@headius they have an optaref instr in yarv which will not dyndispatch in hash case for []. Have you timed this bench using this change with MRI? |
@enebo Bah, you're right. https://bugs.ruby-lang.org/issues/10754 They are just a bit slower on the bench if it uses They probably will reject my change, but I guess this means we could implement it using |
heh, najs trick - esp. the |
decided to look into that -> realized that I also miss JI for Ruby's
all seem unnecessary + there's no consistency on API ( hopefully I will manage to get this acceptable for Ruby 2.4 (and do not change my mind 🎱) |
@kares Ahh, so you're looking into some set.rb improvements! That's great! Try to make sure you isolate jruby-specific stuff so merging from MRI upstream doesn't get too complicated. I'm not opposed to having a native ext if there's something really hot to go there, but the overall performance of set.rb shouldn't usually be limited by the Ruby code. One place it may be necessary would be synchronization; if we decide we want Set to be thread-safe, adding block-based synchronization as in Monitor would be a significant performance hit. Having some native synchronization would help there. Of course, I still hold out hope that some day we might have thread-safe Array and Hash anyway. |
Oh one other thought about the Of course we still plan to find a way to lazily allocate space for backref only when it's needed, but for now static compiler tricks are not bad. |
I wonder if we could do this another way? @lookup = Hash.method(:[])
#...
def include?(element)
@lookup.call(@hash, element)
end I don't neccesarily mean using unbound method it could be our own API which returns a callable thing but we will know what it is because it is just a method. IR could then know for our thing what it is and make it into a static dispatch. |
I am still interested in going native (full set.rb rewrite in Java). set.rb is not changing that much (and isn't actually used beyond some level -> surprised to no find any what I mostly desire - beyond performance - is |
looks promising, 9.1.6.0 (set.rb) :
... native Set/SortedSet (without call site-caching) :
its roughly ~ the same for max compatibility (e.g. has the |
has been delivered in 9.2.0.0 |
... seems that Java's
HashSet
is constantly twice as fast oninclude?
these days :NOTE: Java's Set#include? used to be slow but has been optimized along the 9K line
The text was updated successfully, but these errors were encountered: