Scala Programming Language
  1. Scala Programming Language
  2. SI-6440

Stream#filterNot is too eager after LinearSeqOptimized changes

    Details

      Description

      Moved five methods from LinearSeqOptimized to List

      https://github.com/scala/scala/commit/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.

        Activity

        Hide
        Grzegorz Kossakowski added a comment -

        That looks like pretty bad regression and blocker for 2.10.0.

        Show
        Grzegorz Kossakowski added a comment - That looks like pretty bad regression and blocker for 2.10.0.
        Hide
        Grzegorz Kossakowski added a comment -

        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?

        Show
        Grzegorz Kossakowski added a comment - 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?
        Hide
        Jason Zaugg added a comment -

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

        Show
        Jason Zaugg added a comment - I would recommend that course of action. It would be useful to run your performance rig before and after to see how HotSpot reacts.
        Hide
        Grzegorz Kossakowski added a comment -

        Yeah, I'll check that. Thanks.

        Show
        Grzegorz Kossakowski added a comment - Yeah, I'll check that. Thanks.
        Hide
        Grzegorz Kossakowski added a comment -

        Jason: actually it turned out that the example you give in this ticket has not been broken by
        https://github.com/scala/scala/commit/2a2d9235

        but by

        https://github.com/scala/scala/commit/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?

        Show
        Grzegorz Kossakowski added a comment - Jason: actually it turned out that the example you give in this ticket has not been broken by https://github.com/scala/scala/commit/2a2d9235 but by https://github.com/scala/scala/commit/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?
        Hide
        Jason Zaugg added a comment -

        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.)

        Show
        Jason Zaugg added a comment - 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.)
        Hide
        Grzegorz Kossakowski added a comment -

        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.

        Show
        Grzegorz Kossakowski added a comment - 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.
        Show
        Josh Suereth added a comment - https://github.com/scala/scala/commit/7a6905dc158a7a543ba3f4ddeeffe538580958d3

          People

          • Assignee:
            Grzegorz Kossakowski
            Reporter:
            Jason Zaugg
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development