Truffle ropes 2 #3619
Closed
Truffle ropes 2 #3619
+1,515
−541
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| if (RubyGuards.isRubyString(string)) { | ||
| return StringOperations.getByteListReadOnly(string).dup(); | ||
| // TODO (nirvdrum 25-Jan-16) Should we flatten the rope to avoid caching a potentially deep rope tree? |
There was a problem hiding this comment.
But then you would lose Rope identity comparison, it's a trade-off.
There was a problem hiding this comment.
Indeed. Although we're still doing a full byte[] comparison if the ropes aren't identity equal. We can eliminate that, but I was just maintaining existing functionality for now. The flattening could also work in conjunction with a lookup table to ensure the same references, as well.
|
I read through the diff quickly, looks fine 😄 |
…alizing a byte[] if not necessary.
…atible encodings.
This prevents a lot of converting Ropes into ByteLists so they can be used by Symbols.
…h codes have already been calculated. This could help avoid a length byte walk.
We probably need something more robust to various attacks, but we definitely shouldn't be creating a temporary ByteList to get a hash code.
…filling each descendent's byte[] as well.
* Unified with the String#data method. * Used Rubinius::ByteArray and removed our Rubinius::StringData shim. * Cached value in String DynamicObject and invalidate on Rope update.
nirvdrum
force-pushed
the
truffle-ropes-2
branch
from
4e7a3a9 to
5a81cfa
Compare
|
Looks good to me. Can we have a final PR for all changes and a merge commit so it's clear when it went it? |
|
Yeah. This is already rebased on |
|
👍 |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@chrisseaton @pitr-ch @eregon This is the additional work I've done on ropes since the last pull request. I did it as a separate pull request against the branch you've already reviewed so you can see the new changes more easily.
The work here was primarily to improve performance over the original branch. The first one was basically just functionally complete. This PR has a larger emphasis on doing things more efficiently with ropes. There's still a lot more we can do, particularly in the area of regular expressions.
Notable highlights:
Rope#hashCodeandRope#equals.