-
-
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
Recursion detection should use identity map, but it blows up #3887
Labels
Milestone
Comments
Fixed the diff to include the change in Ruby.java. |
headius
added a commit
that referenced
this issue
May 14, 2016
The old recursion guard (ported from MRI years ago) had a number of flaws: * It forced an object ID for every object encountered. * It created transient objects for every recursive stack. * It was not using identity hashing (#3887). * It used a RubyHash internally, which has much more overhead than a typical JDK Map. The new implementation largely follows the pattern of the original but fixes all the above items. It passes all untagged specs. See also #3884, which started this whole thing.
headius
added a commit
that referenced
this issue
May 14, 2016
The old recursion guard (ported from MRI years ago) had a number of flaws: * It forced an object ID for every object encountered. * It created transient objects for every recursive stack. * It was not using identity hashing (#3887). * It used a RubyHash internally, which has much more overhead than a typical JDK Map. The new implementation largely follows the pattern of the original but fixes all the above items. It passes all untagged specs. See also #3884, which started this whole thing.
This will be done when we merge |
headius
added a commit
that referenced
this issue
May 17, 2016
The old recursion guard (ported from MRI years ago) had a number of flaws: * It forced an object ID for every object encountered. * It created transient objects for every recursive stack. * It was not using identity hashing (#3887). * It used a RubyHash internally, which has much more overhead than a typical JDK Map. The new implementation largely follows the pattern of the original but fixes all the above items. It passes all untagged specs. See also #3884, which started this whole thing.
headius
added a commit
that referenced
this issue
May 18, 2016
The old recursion guard (ported from MRI years ago) had a number of flaws: * It forced an object ID for every object encountered. * It created transient objects for every recursive stack. * It was not using identity hashing (#3887). * It used a RubyHash internally, which has much more overhead than a typical JDK Map. The new implementation largely follows the pattern of the original but fixes all the above items. It passes all untagged specs. See also #3884, which started this whole thing.
This merged to master a while back. The new Ruby.safeRecurse API works like execRecursive, but without being a direct port from MRI. This means it doesn't use Ruby collections and it has cut out many Rubyisms that were not necessary on JRuby. It also uses an IdentityHashMap. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I believe our "execRecursive" logic (mostly ported from MRI) should be using an identity hash. This appears to be what MRI does, and it makes sense since calculating a full hash during a recursive walk could be O(something really big).
However, when I attempted to apply the following patch, recursion detection seemed to break altogether:
We really need to fix this up the right way.
The text was updated successfully, but these errors were encountered: