misc: remove scalaxy, use spire.cforRange

[szymonm]

Pull Request checklist

• The commit(s) message(s) follows the contribution guidelines ?
• Tests for the changes have been added (for bug fixes / features) ?
• Docs have been added / updated (for bug fixes / features) ?

Current behavior :

Scalaxy is not released for Scala 2.12, so I'm migrating the codebase to use spire.cforRange (see #457 for more context).

Runs of jmh benchmarks showed that the performance is the same.

If you have any E2E benchmarks, it would be good to run them to verify consistency between synthetic benchmarks and real life...

[broneill]

Nice job! Looks like one case was missed, however.

I also decided to look at the compiled code to see the difference between the old and new code, and it actually looks a bit better with the new cfor macro.

[broneill]

Missed a case here. Should use cforRange.

[szymonm]

Good catch!

[szymonm]

Went again through all the changes and didn't spot any more missed cases.

[velvia]

@broneill would love if you posted details on the generated code and differences, just curious….

…

On Aug 25, 2020, at 11:46 AM, Szymon Matejczyk ***@***.***> wrote: @szymonm commented on this pull request. In memory/src/main/scala/filodb.memory/format/UnsafeUtils.scala <#866 (comment)>: > @@ -145,7 +143,7 @@ object UnsafeUtils { if (wordComp == 0) { var pointer1 = offset1 + minLenAligned var pointer2 = offset2 + minLenAligned - for { i <- minLenAligned until minLen optimized } { + for { i <- minLenAligned until minLen } { Went again through all the changes and didn't spot any more missed cases. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#866 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIDPW6WGR6XID72BK53EVDSCQBH7ANCNFSM4QGSHL3Q>.

[broneill]

Here's an example from CompositeOrdering.compare method. Before:

    public int compare(scala.collection.Seq, scala.collection.Seq) {
       <snip>
        iconst_0
        istore_3 // start$macro$2: int
        aload_1 // x: scala.collection.Seq
        invokeinterface int scala.collection.Seq.length()
        istore 4 // end$macro$3: int
        iload_3 // start$macro$2: int
        istore 5 // i$macro$4: int
    L1_46:
        iload 5 // i$macro$4: int
        iload 4 // end$macro$3: int
        if_icmpge L3_119
        iload 5 // i$macro$4: int
        istore 6 // iVal$macro$5: int
        <snip>
    L2_110:
        iload 5 // i$macro$4: int
        iconst_1
        iadd
        istore 5 // i$macro$4: int
        goto L1_46

After:

    public int compare(scala.collection.Seq, scala.collection.Seq) {
        <snip>
        iconst_0
        istore_3 // index$macro$3: int
        aload_1 // x: scala.collection.Seq
        invokeinterface int scala.collection.Seq.length()
        istore 4 // limit$macro$5: int
    L1_43:
        iload_3 // index$macro$3: int
        iload 4 // limit$macro$5: int
        if_icmpge L3_106
        <snip>
    L2_99:
        iload_3 // index$macro$3: int
        iconst_1
        iadd
        istore_3 // index$macro$3: int
        goto L1_43

I omitted the code that stayed the same. The important parts are the "*$macro" local variables, which control the loop in the usual way -- initialize to zero, compare each time, increment each time. The new version uses less local variables, and so it's a bit better. In practice, the HotSpot compiler reduces the use of extra local variables and so the performance of both versions should be the same.