Skip to content
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

Predef.arrayToCharSequence.subSequence copies arrays around leading to bad performance in code relying on subSequence being fast #5641

Closed
scabug opened this issue Apr 2, 2012 · 3 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Apr 2, 2012

The concrete problem is that CharArrayReader's constructor implicitly uses Predef.arrayToCharSequence which implements CharSequence.subSequence by calling Array.slice on the underlying array which creates a new copy of the array containing the subsequence. Contrast this with String.subSequence which creates a shallow copy with only updated indices.

Using a CharArrayReader in parser combinators can therefore lead to really bad performance issues which can't be figured out easily.

The next question is if Predef.arrayToCharSequence really should implement subsequence by making a copy. The problem is that java.lang.CharSequence isn't explicit in what to expect from subSequence if the underlying data structure is mutable.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 2, 2012

Imported From: https://issues.scala-lang.org/browse/SI-5641?orig=1
Reporter: @jrudolph
Affected Versions: 2.9.1

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Apr 4, 2012

@jrudolph said:
Martin's comment on that:

On Tue, Apr 3, 2012 at 2:25 AM, martin odersky <martin.odersky@epfl.ch> wrote:
> On Mon, Apr 2, 2012 at 3:51 AM, Johannes Rudolph
>> 1.) Create a shallow copy without copying in `subSequence`. This may
>> break code which relies on the `CharSequence` returned from
>> `subSequence` to be unchanged if the underlying array is mutated.
>
> I would vote for 1). Since the arrayToCharSequence does not copy the whole
> array when converting to a CharSequence, any arguments based on mutability
> are moot. So I would classify this as a (performance) bug, which needs to be
> fixed.
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented May 5, 2012

@paulp said:
b5e9e4b950

@scabug scabug closed this May 5, 2012
@scabug scabug added this to the 2.10.0 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.