-
-
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
[Truffle] Iterative implementations for getByteSlow #4201
Conversation
So this is just avoiding recursive calls in case of largely homegeneous ropes, right? |
@eregon Yes, I did this to avoid stack overflow error. For example, it fixes stack overflow for: I'm not sure about performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is right, but unfortunately you still recurse in various places. If you managed to have a tree made up of alternating ConcatRope
and SubstringRope
, you'd wind up with as many calls here as you had with the old approach.
And the ConcatRope
case only works well for trees that are biased to the left. That happens to be a large number of our trees, but if we have one that goes the other way (i.e., heavy down the right), you'd still recurse.
return child.getByteSlow(index + offset); | ||
int idx = index + offset; | ||
Rope nextChild = child; | ||
while (nextChild instanceof SubstringRope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there's nothing wrong with this check, when we actually create SubstringRope
instances we always collapse substrings of substrings (see
jruby/truffle/src/main/java/org/jruby/truffle/core/rope/RopeNodes.java
Lines 119 to 124 in 5c97ec0
@Specialization(guards = { "byteLength > 1", "!sameAsBase(base, byteLength)" }) | |
public Rope substringSubstringRope(SubstringRope base, int offset, int byteLength, | |
@Cached("createBinaryProfile()") ConditionProfile is7BitProfile, | |
@Cached("createBinaryProfile()") ConditionProfile isBinaryStringProfile) { | |
return makeSubstring(base.getChild(), offset + base.getOffset(), byteLength, is7BitProfile, isBinaryStringProfile); | |
} |
And since you only handle that one case here, I think whatever you were looking to accomplish devolves into the code you replaced.
Going to decline this for now because: it is not a complete solution, code in this area is expected to change soon, and it is not currently needed for gem installing with a workaround in place. |
No description provided.