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
SI-5017 Poor performance of :+ operator on Arrays #1739
Conversation
result(0) = elem | ||
Array.copy(repr, 0, result, 1, repr.length) | ||
result | ||
} |
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.
Is this code called from any existing tests? (Replace it with ???
and run partest to see.)
If so, mention that test in the commit comment; if not, please add a test.
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1830/ |
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1830/ |
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1118/ |
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1118/ |
After addressing the issue of missing tests, LGTM. |
please use the SI-5017 naming convention |
Control performance of :+ and +: operator on my machine were 700-800 ms After adding size hint on the implementation in SeqLike, it went down to 500-600 ms But with specialixed implementation in ArrayOps, brings it down to 300-400 ms Unfortunatly, this method will only be called when the Array object is being referenced directly as it's type, but that should be the case enough times to justify the extra method. I ended up removing the sizeHint in SeqLike because it made the execution of the "benchmark" slower when the Array was being manipulated as a Seq. Side note: Interestingly enough, the benchmark performed better on my virtualized Fedora 17 with JDK 7 than natively on Mac OS X with JDK 6
Sorry for the delay. Got caught up finishing work stuff. |
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1222/ |
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/1222/ |
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1938/ |
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1938/ |
Looks like all the discussion has been addressed and LGTM |
SI-5017 Poor performance of :+ operator on Arrays
Control performance of :+ and +: operator on my machine were 700-800 ms
After adding size hint on the implementation in SeqLike, it went down to 500-600 ms
But with specialixed implementation in ArrayOps, brings it down to 300-400 ms
Unfortunatly, this method will only be called when the Array object is being referenced directly as it's type, but that should be the case enough times to justify the extra method.
I ended up removing the sizeHint in SeqLike because it made the execution of the "benchmark" slower when the Array was being manipulated as a Seq.
Side note: Interestingly enough, the benchmark performed better on my virtualized Fedora 17 with JDK 7 than natively on Mac OS X with JDK 6
Review by @axel22