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

More efficient ArraySeq iteration #8300

Open
wants to merge 1 commit into
base: 2.13.x
from

Conversation

@szeiger
Copy link
Contributor

commented Aug 2, 2019

  • Use ArrayIterator for ArraySeq: As discovered in
    scala/bug#11658, ArraySeq (both mutable
    and immutable) used the generic IndexedSeqView-based iterators even
    though the more efficient ArrayIterator is available.

  • Avoid runtime type checks for ArraySeq steppers: ArraySeq.stepper
    had to perform a runtime type check for the array. Now there are
    separate implementations for all specialized ArraySeq types that
    avoid the extra checks.

  • Add a useful knownSize implementation to ArrayIterator.

@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Aug 2, 2019

@lrytz

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

+java.io.NotSerializableException: scala.collection.ArrayOps$ArrayIterator$mcI$sp

in test/files/run/serialize-stream.scala

@lrytz
Copy link
Member

left a comment

Stepper change LGTM

@szeiger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

The serialization change is problematic. ArrayIterator (along with the other array-specific iterator implementations) is not serializable. It should have been. Making it serializable now is not forwards binary compatible, suddenly returning a non-serializable implementation is not strictly incompatible (since we never promised that it would be serializable) but can break code (as observed in the test failure).

@lrytz

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

We could have a private, Serailizable subclass of ArrayIterator and use that (without exposing its static type).

Is there a general story / spec about iterators being serializable?

@szeiger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Hm, right. On second thought, making those classes Serializable should be OK. They are private anyway.

@lrytz

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Ah right, ArrayIterator is private, so even better.

@szeiger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

The MiMa errors are strange:

* abstract method stepper(scala.collection.StepperShape)scala.collection.Stepper in class scala.collection.immutable.ArraySeq does not have a correspondent in current version
* in current version there is abstract method stepper(scala.collection.StepperShape)scala.collection.Stepper in class scala.collection.immutable.ArraySeq, which does not have a correspondent
* abstract method stepper(scala.collection.StepperShape)scala.collection.Stepper in class scala.collection.mutable.ArraySeq does not have a correspondent in current version
* in current version there is abstract method stepper(scala.collection.StepperShape)scala.collection.Stepper in class scala.collection.mutable.ArraySeq, which does not have a correspondent

The signatures are the same but it complains about the methods both ways.

(And contrary to what I originally suspected, relaxing accessibility of ArrayIterator is not a binary compatibility issue at all because it ends up being public in bytecode either way)

@szeiger szeiger force-pushed the szeiger:wip/arrayseq-iterators branch 2 times, most recently from 91e0b9e to 4daa59b Aug 5, 2019

More efficient ArraySeq iteration
- Use `ArrayIterator` for `ArraySeq`: As discovered in
  scala/bug#11658, `ArraySeq` (both mutable
  and immutable) used the generic `IndexedSeqView`-based iterators even
  though the more efficient `ArrayIterator` is available.

- Avoid runtime type checks for `ArraySeq` steppers: `ArraySeq.stepper`
  had to perform a runtime type check for the array. Now there are
  separate implementations for all specialized `ArraySeq` types that
  avoid the extra checks.

- Add a useful `knownSize` implementation to `ArrayIterator`.
@szeiger

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

I examined the signatures manually. The MiMa errors are confusing and the wording of the first one ("abstract method ... does not have a correspondent in current version") is plain wrong. The actual change is as follows:

Old (concrete stepper implementation in ArraySeq):

  public <S extends scala.collection.Stepper<?>> S stepper(scala.collection.StepperShape<A, S>);
    descriptor: (Lscala/collection/StepperShape;)Lscala/collection/Stepper;

New (abstract stepper declaration):

  public abstract <S extends scala.collection.Stepper<?>> S stepper(scala.collection.StepperShape<A, S>);
    descriptor: (Lscala/collection/StepperShape;)Lscala/collection/Stepper;

So whereas the first error message gives the impression that the old version had an abstract method, it is in fact the new version.

We could leave out the abstract declaration to satisfy MiMa but it wouldn't be source compatible. It exists to refine the return type from S to S with EfficientSplit (which is erased in bytecode). Since ArraySeq is sealed, it doesn't matter if the method is abstract or concrete.

@szeiger szeiger force-pushed the szeiger:wip/arrayseq-iterators branch from 4daa59b to 946414c Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.