-
Notifications
You must be signed in to change notification settings - Fork 3.1k
SI-8589 Performance improvement for ArrayCharSequence.toString #3752
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
Conversation
|
Review by @Ichoran |
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.
This has different behavior when the bounds are incorrect. The old version would return an empty string. The new one throws StringIndexOutOfBoundsException. Unless there's a compelling reason to prefer the latter, you should check the bounds. (The class itself does not.) (In particular, start >= 0, start+length <= s.length.)
|
Thanks for the review, and sorry for missing that. I assumed the bounds would be ok. override def toString = { } And tested that with this: val chars = "jdfhfjhdfkjasdfjkdsajkfasdjkfhasdkjfhsdafkj48r848rweruqewiuroqweoiruiwqeruoiwe".toCharArray val ok = (1 to 10000000).forall { _ => } I am a bit new to this, can i just commit the change to my branch (i did already) or do i need to rebase it into a single commit? |
|
It is preferable to rebase into a single commit (git rebase -i HEAD~2 should do the trick). |
|
ping @Ichoran |
|
I tried rebasing it but i had to merge my remote branch again creating an extra empty commit. Not sure why... I hope it is ok like this. |
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.
Use math not Math
|
@jeroentervoorde - You should be able to get a clean rebase by following these steps, assuming you've got "origin" as your remote github repository and "scala" as the main Scala one (let me know if this doesn't make sense to you; note that we normally name the branches issue/8589 not SI_8589): (1) checkout your 2.11.x branch: git checkout 2.11.x |
|
I think it is ok now. Thanks very much for your help. I'm off reading the git book again :) |
|
That part looks good. Any chance you can do it one more time to use |
|
Sorry, i missed that. It's updated now. |
|
ping @Ichoran |
|
@adriaanm - Whoops, overlooked this, sorry! LGTM |
|
No worries -- please start your comment with LGTM when the LGTM is final -- that's what triggers the bot to add the "reviewed" label. |
SI-8589 Performance improvement for ArrayCharSequence.toString
|
Thanks for the improvement, @jeroentervoorde! |
While using a csv parser to process large char arrays i ran into the problem that the toString method of ArrayCharSequence was very slow on large buffers (and, as it appears, also on small buffers)
The problematic method:
override def toString = xs drop start take length mkString ""
I created a faster version which is already almost 50% faster on an empty charsequence and only becomes relatively faster when the array gets bigger.
See https://issues.scala-lang.org/browse/SI-8589