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 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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

scabug commented May 5, 2012

@paulp said:
b5e9e4b950

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants