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

List(...) builder missing Scala 2 optimization to avoid intermediate array #17035

Closed
armanbilge opened this issue Mar 3, 2023 · 4 comments
Closed
Assignees
Labels

Comments

@armanbilge
Copy link
Contributor

Compiler version

3.3.1-RC1-bin-20230301-0df5ae2-NIGHTLY

Minimized code

class Test {
  def foo = List("foo", "bar", "baz")
}

Output

/*
 * Decompiled with CFR 0.151.
 */
import scala.collection.immutable.List;
import scala.collection.immutable.Seq;
import scala.package$;
import scala.runtime.ScalaRunTime$;

public class Test {
    public List<String> foo() {
        return (List)package$.MODULE$.List().apply((Seq)ScalaRunTime$.MODULE$.wrapRefArray((Object[])new String[]{"foo", "bar", "baz"}));
    }
}

Expectation

The List is built directly, without going through an intermediate varargs array.

Scala 2.13.10 decompiles to this:

/*
 * Decompiled with CFR 0.151.
 */
import scala.collection.immutable.;
import scala.collection.immutable.List;
import scala.collection.immutable.Nil$;

public class Test {
    public List<String> foo() {
        return new .colon.colon((Object)"foo", (List)new .colon.colon((Object)"bar", (List)new .colon.colon((Object)"baz", (List)Nil$.MODULE$)));
    }
}
@armanbilge armanbilge added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 3, 2023
lihaoyi added a commit to com-lihaoyi/fastparse that referenced this issue Mar 3, 2023
1. Previously `.rep()` never inlined, and `.repX()` would always inline
and fail with a compile error if it was not possible. This should make
both perform inlining at a best-effort level, giving us the best of both
worlds

2. I saw a lot of calls to `List()` turning up in my JProfile, replaced
them with raw `::` calls. Seems like this is optmized automatically in
Scala 2, but not in Scala 3
scala/scala3#17035

3. Lift whitespace NoWhitespace check in `.rep` to compile time, like we
do in `~`.

4. Combine all the menagerie of `.rep` implementations into one Scala 3
macro. Scala 2 is still a bit of a mess, cleaning that up is left for
future

6. Inlined the character checking in `literalStrMacro` to avoid
allocating an intermediate function to call

Seems to provide a small but measurable performance boost for successful
parses, and a significant performance boost for failures (where we
allocate a lot more lists as part of error reporting). Not quite enough
to catch up to Scala 2 performance, but brings us maybe half way there.

Scala 3 before:

```
JsonnetParse Benchmark
Max time - 10000 ms. Iterations - 5.
Iteration 1
Benchmark 0. Result: 9331
Benchmark 1. Result: 1546
Iteration 2
Benchmark 0. Result: 7715
Benchmark 1. Result: 1947
Iteration 3
Benchmark 0. Result: 7549
Benchmark 1. Result: 1976
Iteration 4
Benchmark 0. Result: 7613
Benchmark 1. Result: 1953
Iteration 5
Benchmark 0. Result: 7686
Benchmark 1. Result: 1907
```

Scala 3 after:

```
JsonnetParse Benchmark
Max time - 10000 ms. Iterations - 5.
Iteration 1
Benchmark 0. Result: 9466
Benchmark 1. Result: 2611
Iteration 2
Benchmark 0. Result: 8152
Benchmark 1. Result: 2832
Iteration 3
Benchmark 0. Result: 8139
Benchmark 1. Result: 2799
Iteration 4
Benchmark 0. Result: 8020
Benchmark 1. Result: 2844
Iteration 5
Benchmark 0. Result: 8126
Benchmark 1. Result: 2868
```

Scala 2.13:

```
JsonnetParse Benchmark
Max time - 10000 ms. Iterations - 5.
Iteration 1
Benchmark 0. Result: 9850
Benchmark 1. Result: 2773
Iteration 2
Benchmark 0. Result: 8781
Benchmark 1. Result: 2912
Iteration 3
Benchmark 0. Result: 8742
Benchmark 1. Result: 2916
Iteration 4
Benchmark 0. Result: 8782
Benchmark 1. Result: 2912
Iteration 5
Benchmark 0. Result: 8703
Benchmark 1. Result: 2940
```
@smarter smarter added itype:enhancement area:transform itype:performance and removed itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Mar 5, 2023
@smarter
Copy link
Member

smarter commented Mar 5, 2023

For anyone interested in giving this a try, this should probably be done done as a mini-phase similar to https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/dotc/transform/TupleOptimizations.scala.

@nicolasstucki
Copy link
Contributor

It is better to do this one after erasure. The Array.apply transform seems to be more closely related to this optimization https://github.com/lampepfl/dotty/blob/main/compiler/src/dotty/tools/dotc/transform/ArrayApply.scala.

@smarter smarter added the Spree Suitable for a future Spree label Mar 6, 2023
@scala-center-bot
Copy link

scala-center-bot commented Mar 21, 2023

This issue was picked for the Issue Spree No. 28 of 28 March 2023 which takes place in a week from now. @smarter, @Decel, @KuceraMartin will be working on it. If you have any insight into the issue or guidance on how to fix it, please leave it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants