Improve implicit search times for Filter, FilterNot, Union, Intersection #682

aryairani · 2017-01-22T21:51:02Z

Hi,

This patch provides a much faster implementation of Filter and FilterNot, and somewhat faster implementations of Union and Intersection. (Without the patch, my compile times have been prohibitively slow, such that I can only use the static checks for very small operations.)

(Note the log-lin scale.)

Codecov check fails only because newly-deprecated code (retained only for bincompat) is no longer used by tests. The non-covered code can be be deleted at the next minor release.

codecov-io · 2017-01-22T23:08:31Z

Current coverage is 81.35% (diff: 71.42%)

Merging #682 into master will decrease coverage by 0.20%

@@             master       #682   diff @@
==========================================
  Files            69         69          
  Lines          2641       2655    +14   
  Methods        2375       2392    +17   
  Messages          0          0          
  Branches         92         89     -3   
==========================================
+ Hits           2154       2160     +6   
- Misses          487        495     +8   
  Partials          0          0

Powered by Codecov. Last update b94bbf2...0006ac9

milessabin · 2017-01-24T14:09:09Z

Can you add tests which explicitly invoke the now redundant methods?

aryairani · 2017-01-24T15:00:56Z

Sure, I will take a look at that.

aryairani · 2017-01-26T17:04:32Z

Hi — I could use guidance on two issues:

In scalacOptions we have -deprecation and -Xfatal-warnings; with the two together, I can't include tests for deprecated methods. I could not deprecate them, but I think they should be deprecated.
I noticed that the hlist syntax methods .filter and .filterNot directly summon a Partition[L,U] instead of Filter[L,U] / FilterNot[L,U], which seems like a mistake. I'm not sure how to fix these in a bincompatible way. I could:

a. leave them as-is (linked to the wrong typeclass) or
b. move them to a HListOpsDeprecated trait (this builds, but not sure how it affects bc yet; I havent been able to run mima tests locally; don't know why.)

milessabin · 2017-02-10T11:44:15Z

The old methods won't appear explicitly in any normal user code because they're intended to be invoked implicitly. That being so I think it's right to leave them undeprecated.

A couple of thoughts. Can you get the same improvements for Partition? In which case it might be better to leave Filter and FilterNot as they are, which would also solve the .filter and .filterNot problem.

Also could you benchmark this change against leaving things as they are but compiling with Typelevel Scala 2.12.1 with the -Yinduction-heuristics flag enabled?

aryairani · 2017-02-11T18:28:15Z

With respect to @deprecation, I'm happy to replace the annotation with a comment; but remember, the issue with deprecation only came up because codecov wants us to continue referencing the unused methods from tests. You want them to go away eventually, right?
This particular improvement won't help Partition, because Partition by its contract needs to compute both the Filter and FilterNot results. The current implementation of Filter and FilterNot rely on Partition to compute both sets, and then discard one of them. But computing both seems to be way more expensive than just computing the one that's needed.

We could probably flip things around so that Partition relies on the new Filter and FilterNot, instead of the other way around, and end up with a linear slowdown rather exponential. This would "help" the .filter and .filterNot issues somewhat, although it still seems screwy that those methods take a different type class than the ones they're named after. Maybe the methods predate the corresponding type classes, otherwise I dunno how it came to be the way it is. They should really use their corresponding type classes though.

I will benchmark against -Yinduction-heuristics when I am home next week. If it's a huge help, then one option would be to forget this, and leave Lightbend folks to suffer; what do you think?

milessabin · 2017-02-13T07:59:03Z

On (1) yes, agreed.

On (2), my intuitions are that if your rework of Filter/FilterNot can improve the compile time from quadratic to linear, then something similar should be possible for Partition. I agree that Partition will still be more expensive to compile than either of the two individually, but the margin might be small enough that the benefit of having a single implementation rather than three might outweigh it.

On (3) I think it makes sense to do this whether or not -Yinduction-heurisitics improves things, but it would be very useful to see if it helps directly in this case.

aryairani · 2017-02-14T14:42:40Z

So on (1), would you prefer to (1a) not include the deprecated methods in tests, and ignore the codecov result, (1b) allow deprecated methods in tests by removing -Xfatal-warnings in tests, or (1c) not use the @deprecation annotation, but just leave a comment that the methods are deprecated?

On (2), I can get it to two implementations, rather than three, but not down to one. Partition is inferring two HLists simultaneously; the rework for Filter/FilterNot was merely to cut them down to each only infer one HList. I can't apply the same rework to Partition because it needs to produce both. However, I can invert the dependence and have Partition use Filter/FilterNot to infer the lists sequentially instead of simultaneously. Edit: I tried this and surprisingly it didn't help at all (green line).

With respect to -Yinduction-heuristics, it unfortunately seems to make things worse; roughly equivalent to +1 test size. (purple line)

Finally, I think I will move the simpler Union/Intersection part of this PR to a separate PR, because that patch won't cause bincompat issues, so can probably be merged sooner. Still requires choosing among (1a), (1b), (1c).

Thanks for reading this far ;-)

milessabin · 2017-02-14T15:12:07Z

(1c) ... thanks :-)

milessabin · 2017-12-18T15:03:48Z

@refried could you and @joroKr21 work on combining this PR with #795.

joroKr21 · 2017-12-19T17:48:19Z

I don't think #795 is any faster. I hadn't realized FilterNot was implemented in such a weird way. This is a simpler and more general solution 👍

core/src/main/scala/shapeless/ops/hlists.scala

joroKr21 · 2019-12-14T14:41:49Z

@aryairani it's been long time. Sorry about that. We are finally looking at a 2.4.0 release soon and I think this would be an important part of it. But the PR needs a rebase to resolve conflicts.

joroKr21 · 2019-12-29T22:54:04Z

actually might make sense to check scala 2.12.10 / 2.13.1 first

aryairani · 2019-12-30T04:56:39Z

I"m assuming we're no longer worried about bincompat for the purposes of this PR (due to the major version update)? I see some of the surrounding methods have already seen their signatures changed.

joroKr21 · 2019-12-30T06:44:34Z

Yes, that's right we are breaking bincompat for 2.4.0

aryairani · 2019-12-30T07:32:01Z

Okay, I cleaned it up a bit.
sbt validate passed locally for me; let's see what CI says.

joroKr21 · 2020-01-12T21:34:00Z

@aryairani thanks for coming back to this. Do you have the original code you used to test the performance? I want to try with newer versions of Scala to see if the performance gains are as dramatic as before.

aryairani · 2020-01-21T03:08:03Z

@joroKr21 Sorry for the delay —

That's a bit tough 3 years later. 😅 I don't seem to have code immediately accessible, i.e. haven't found it in Dropbox. The computer I authored it and ran it on is currently in storage, so I can't easily check it. I may have a backup of said machine in the house, on an external backup appliance that's currently boxed up in the basement.

How badly do we want that original code? Inventing something from scratch might be easier.

WDYT?

joroKr21 · 2020-01-21T03:26:17Z

Ah, don't go through so much trouble. will try something from scratch.

joroKr21 · 2020-03-15T11:39:03Z

@aryairani I finally found time to try out the performance. Tried with an HList with 100 elements:

  type B = Boolean
  type I = Int
  type L =
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    B :: I :: B :: I :: B :: I :: B :: I :: B :: I ::
    HNil
  Partition[L, I]

I think we can optimise Partition and leave Filter and FilterNot as they are.
The key observation is the difference between:

2 seconds:

    implicit def hlistPartition2[H, L <: HList, U, LPrefix <: HList, LSuffix <: HList](
      implicit e: U =:!= H, p: Aux[L, U, LPrefix, LSuffix]
    ): Aux[H :: L, U, LPrefix, H :: LSuffix]

Too long, didn't wait:

    implicit def hlistPartition2[H, L <: HList, U, LPrefix <: HList, LSuffix <: HList](
      implicit p: Aux[L, U, LPrefix, LSuffix], e: U =:!= H
    ): Aux[H :: L, U, LPrefix, H :: LSuffix]

joroKr21 · 2020-03-15T11:41:10Z

Do you have time to make that change?

Putting the negation condition first improves dramatically.

joroKr21

Added a commit to optimise Partition instead.

aryairani · 2020-03-16T17:27:31Z

So wait, then is there no performance difference between the new Partition and the proposed Filter/FilterNot?

I missed a comparison in #682 (comment).

joroKr21 · 2020-03-16T17:29:17Z

Maybe there is a small difference but nothing noticeable (i.e. all are linear now).

aryairani · 2020-03-16T17:31:08Z

Okay, great.

milessabin added this to the shapeless-2.4.0 milestone Dec 18, 2017

joroKr21 reviewed Dec 19, 2017

View reviewed changes

core/src/main/scala/shapeless/ops/hlists.scala Outdated Show resolved Hide resolved

joroKr21 reviewed Dec 19, 2017

View reviewed changes

core/src/main/scala/shapeless/ops/hlists.scala Outdated Show resolved Hide resolved

joroKr21 mentioned this pull request Dec 19, 2017

[WIP] Faster HList Intersection #795

Closed

chrisbenincasa mentioned this pull request Oct 23, 2018

Performance issue with HList's Filter #596

Closed

Improve implicit search times for Filter, FilterNot, Union, Intersection

6985c2d

aryairani force-pushed the faster-union-intersection branch from 0006ac9 to 6985c2d Compare December 30, 2019 07:31

joroKr21 added 2 commits March 15, 2020 17:47

Merge branch 'master' into faster-union-intersection

36a1aee

Optimize Partition and revert to old Filter and FilterNot

683b1ee

Putting the negation condition first improves dramatically.

joroKr21 force-pushed the faster-union-intersection branch from cffd7f4 to 683b1ee Compare March 15, 2020 17:01

joroKr21 approved these changes Mar 15, 2020

View reviewed changes

joroKr21 assigned aryairani Mar 15, 2020

joroKr21 merged commit 2a6e8eb into milessabin:master Mar 15, 2020

joroKr21 mentioned this pull request Mar 16, 2020

Intersection is too slow for case class with more than 6 fields #793

Closed

joroKr21 mentioned this pull request Mar 21, 2020

Compiler hangs when using LabelledGeneric to convert classes #706

Closed

joroKr21 mentioned this pull request Apr 21, 2021

Improve implicit search times for Filter, FilterNot, Union, Intersection #1114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve implicit search times for Filter, FilterNot, Union, Intersection #682

Improve implicit search times for Filter, FilterNot, Union, Intersection #682

aryairani commented Jan 22, 2017 •

edited

Loading

codecov-io commented Jan 22, 2017

milessabin commented Jan 24, 2017

aryairani commented Jan 24, 2017

aryairani commented Jan 26, 2017 •

edited

Loading

milessabin commented Feb 10, 2017

aryairani commented Feb 11, 2017 •

edited

Loading

milessabin commented Feb 13, 2017

aryairani commented Feb 14, 2017

milessabin commented Feb 14, 2017

milessabin commented Dec 18, 2017

joroKr21 commented Dec 19, 2017

joroKr21 commented Dec 14, 2019

joroKr21 commented Dec 29, 2019

aryairani commented Dec 30, 2019 •

edited

Loading

joroKr21 commented Dec 30, 2019

aryairani commented Dec 30, 2019 •

edited

Loading

joroKr21 commented Jan 12, 2020

aryairani commented Jan 21, 2020

joroKr21 commented Jan 21, 2020

joroKr21 commented Mar 15, 2020

joroKr21 commented Mar 15, 2020

joroKr21 left a comment

aryairani commented Mar 16, 2020

joroKr21 commented Mar 16, 2020

aryairani commented Mar 16, 2020

Improve implicit search times for Filter, FilterNot, Union, Intersection #682

Improve implicit search times for Filter, FilterNot, Union, Intersection #682

Conversation

aryairani commented Jan 22, 2017 • edited Loading

codecov-io commented Jan 22, 2017

Current coverage is 81.35% (diff: 71.42%)

milessabin commented Jan 24, 2017

aryairani commented Jan 24, 2017

aryairani commented Jan 26, 2017 • edited Loading

milessabin commented Feb 10, 2017

aryairani commented Feb 11, 2017 • edited Loading

milessabin commented Feb 13, 2017

aryairani commented Feb 14, 2017

milessabin commented Feb 14, 2017

milessabin commented Dec 18, 2017

joroKr21 commented Dec 19, 2017

joroKr21 commented Dec 14, 2019

joroKr21 commented Dec 29, 2019

aryairani commented Dec 30, 2019 • edited Loading

joroKr21 commented Dec 30, 2019

aryairani commented Dec 30, 2019 • edited Loading

joroKr21 commented Jan 12, 2020

aryairani commented Jan 21, 2020

joroKr21 commented Jan 21, 2020

joroKr21 commented Mar 15, 2020

joroKr21 commented Mar 15, 2020

joroKr21 left a comment

Choose a reason for hiding this comment

aryairani commented Mar 16, 2020

joroKr21 commented Mar 16, 2020

aryairani commented Mar 16, 2020

aryairani commented Jan 22, 2017 •

edited

Loading

aryairani commented Jan 26, 2017 •

edited

Loading

aryairani commented Feb 11, 2017 •

edited

Loading

aryairani commented Dec 30, 2019 •

edited

Loading

aryairani commented Dec 30, 2019 •

edited

Loading