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

Jump to bottom

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
@gkossakowski
Milestone
2.10.0-RC2

Comments

@scabug
Copy link

scabug commented Sep 27, 2012 

Moved five methods from LinearSeqOptimized to List

scala/scala@2a2d9235

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

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

scabug commented Sep 28, 2012

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

@scabug
Copy link
Author

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

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

scabug commented Oct 3, 2012

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

@scabug
Copy link
Author

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@2a2d9235

but by

scala/scala@df9f470f

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

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

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 as completed Oct 12, 2012
@scabug
Copy link
Author

scabug commented Oct 12, 2012

@jsuereth said:
scala/scala@7a6905d

@scabug scabug added the critical label Apr 7, 2017
@scabug scabug added this to the 2.10.0-RC2 milestone Apr 7, 2017
@scabug scabug mentioned this issue Apr 7, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gkossakowski gkossakowski

Labels
None yet
Projects
None yet
Milestone
2.10.0-RC2
Development

No branches or pull requests
2 participants
@gkossakowski @scabug