scala icon indicating copy to clipboard operation
scala copied to clipboard

Emit flatter case class equals for high arities

Open som-snytt opened this issue 4 years ago • 4 comments

Reduces stack required from 200K to 160K on my machine for sample case class with 175 elements.

The SOE was in erasure.

som-snytt avatar Dec 06 '21 07:12 som-snytt

Seems this generates some unnecessary boolean boxes by going through BoxesRunTime.equals

...
    ALOAD 0
    INVOKEVIRTUAL C.x4 ()LC;
    ALOAD 4
    INVOKEVIRTUAL C.x4 ()LC;
    ASTORE 8
    DUP
    IFNONNULL L21
    POP
    ALOAD 8
    IFNULL L22
    GOTO L23
   L21
   FRAME FULL [C java/lang/Object I java/lang/Object C C C C C] [C]
    ALOAD 8
    INVOKEVIRTUAL java/lang/Object.equals (Ljava/lang/Object;)Z
    IFEQ L23
   L22
   FRAME SAME
    ICONST_1
    GOTO L24
   L23
   FRAME SAME
    ICONST_0
   L24
   FRAME SAME1 I
    INVOKESTATIC scala/runtime/BoxesRunTime.boxToBoolean (Z)Ljava/lang/Boolean;
    ICONST_0
    INVOKESTATIC scala/runtime/BoxesRunTime.boxToBoolean (Z)Ljava/lang/Boolean;
    INVOKESTATIC scala/runtime/BoxesRunTime.equals (Ljava/lang/Object;Ljava/lang/Object;)Z
    IFEQ L25
    ICONST_0
    IRETURN
...

lrytz avatar Dec 06 '21 13:12 lrytz

Still very drafty; a quickie at bedtime last night. Thanks for the heads up about boxing, I think I made that mistake once a long time ago. The other footnote is that I thought the SOE involved unapply with huge tuples to infer, then saw SOE in erasure with equals. Also I saw repeated SOE, but didn't see where it is caught during transform.

Edit: neglected to say re title, Flattery will get you everywhere.

I remember now that it compares primitives first; but probably should do canEqual before AnyRefs.

However, I had forgotten that this follows up a Haoyism from last May. Someone had bumped a different ticket with a question about the same issue. The current limitation in erasure is that (a&b)&(c&d) still goes through adaptMember which recurses into the qualifier.

Previous failure modes: assigning positions, typed apply, the traverser looking for _root_ (and there must be a better way to do that), as well as erasure adaptMember if it gets that far, according to the other ticket.

som-snytt avatar Dec 06 '21 16:12 som-snytt

@lrytz re-review?

SethTisue avatar Jan 19 '22 03:01 SethTisue

oh

the traverser looking for root (and there must be a better way to do that)

is why I revisited that code. (Visitor pattern pun alert.) @SethTisue

Is the better solution a custom tree node, or perhaps a safer attachment that says, "If you understand this attachment, here is a more efficient representation of this subtree." Because it's clear what we want the backend to do.

Opie says more tests would be swell! In fact, the feature isn't urgent and is actually marginal, but if the outcome is improved tests and improved testing skills, then who wouldn't say yes to that?

som-snytt avatar Jan 19 '22 20:01 som-snytt

unclear to me whose court the 🎾 is in, here?

SethTisue avatar Oct 26 '22 11:10 SethTisue