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

Stream#filterNot is too eager after LinearSeqOptimized changes #6440

Closed
scabug opened this issue Sep 27, 2012 · 9 comments
Closed

Stream#filterNot is too eager after LinearSeqOptimized changes #6440

scabug opened this issue Sep 27, 2012 · 9 comments
Assignees
Milestone

Comments

@scabug
Copy link

@scabug scabug commented Sep 27, 2012

Moved five methods from LinearSeqOptimized to List

scala/scala@2a2d923

List ain't the only LinearSeqOptimized in town: that commit changed the performance and evaluation characteristics of the other subclasses: Stream, Queue, Stack, and MutableList.

scala> Stream.continually(()).filterNot(_ => false).take(2)

I've only highlighted the first example I've found. I really don't see how that change can be allowed -- even if you were to copy/paste the removed methods into those other subclasses, you would have broken any third party code extending this trait. You could take the position that it's just an implementation trait, which would let you remove if from the parents of List.

Update: Regressed and now broken again. See #8627

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 27, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6440?orig=1
Reporter: @retronym
Affected Versions: 2.10.0, 2.11.4
Other Milestones: 2.10.0

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Sep 28, 2012

@gkossakowski said:
That looks like pretty bad regression and blocker for 2.10.0.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 3, 2012

@gkossakowski said:
Jason, I agree with you.

Does it make sense to bring those methods back to {{LinearSeqOptimized}} and keep them in {{List}}? That would fix all of the problems and retain performance benefit for List operations, right?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 3, 2012

@retronym said:
I would recommend that course of action. It would be useful to run your performance rig before and after to see how HotSpot reacts.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 3, 2012

@gkossakowski said:
Yeah, I'll check that. Thanks.

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 9, 2012

@gkossakowski said:
Jason: actually it turned out that the example you give in this ticket has not been broken by
scala/scala@2a2d923

but by

scala/scala@df9f470

The reason is that we no longer delegate to filter (which Stream overrides) but we provide our own implementation based on foreach.

I believe that changes to LinearSeqOptimized are irrelevant for other (non-lazy collections) in a sense that they do not change semantics. Do you agree?

I don't understand why Stream mixes-in LinearSeqOptimized in the first place but this seems to be (also) irrelevant. In order to fix this bug we need to override filterNot in Stream the same way as filter is overriden of revert change to TraversableLike. I think the later is way to go.

WDYT?

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 9, 2012

@retronym said:
I can't find any bugs other than filterNot. So fixing that would be sufficient. But this highlights the fragility of sharing code between lazy and strict collections. (Same story for immutable/mutable.)

@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 9, 2012

@gkossakowski said:
Ok. I'll go and revert change in TraversableLike.

I agree on general comment. However, fixing it seems very tricky as it would require some major changes to collection hierarchy.

@scabug scabug closed this Oct 12, 2012
@scabug

This comment has been minimized.

Copy link
Author

@scabug scabug commented Oct 12, 2012

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.