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

Use StringConcatFactory for string concatenation on JDK 9+ #12929

Merged
merged 3 commits into from
Jul 29, 2021

Conversation

harpocrates
Copy link
Contributor

@harpocrates harpocrates commented Jun 25, 2021

JEP 280, released in JDK 9, proposes a new way to compile string
concatenation using invokedynamic and StringConcatFactory.
This new approach generates less bytecode, doesn't have to incur
the overhead of StringBuilder allocations, and allows users to
pick swap the concatenation technique at runtime.

This changes the codegen when the target is at least Java 9 to
leverage invokedynamic and StringConcatFactory. On Java 8,
the old StringBuilder approach is still used.

Ported from scala/scala#9556

[test_java8]

JEP 280, released in JDK 9, proposes a new way to compile string
concatenation using `invokedynamic` and `StringConcatFactory`.
This new approach generates less bytecode, doesn't have to incur
the overhead of `StringBuilder` allocations, and allows users to
pick swap the concatenation technique at runtime.

This changes the codegen when the target is at least Java 9 to
leverage `invokedynamic` and `StringConcatFactory`. On Java 8,
the old `StringBuilder` approach is still used.

Ported from scala#9556
@anatoliykmetyuk
Copy link
Contributor

@smarter @harpocrates what's the status on this one? Is there more work that needs to be done? Or does this only need a reviewer?

@harpocrates
Copy link
Contributor Author

@anatoliykmetyuk there's two code issues. I think I can do these, but I'd be happy if someone else picked it up (I've forgotten all of Dotty's conventions around testing).

  1. The test needs to be ported over to Dotty conventions
  2. One of the tests is likely to cause a stack overflow during compilation (due to the AST being very deep). @smarter suggested a workaround (which I've not tried yet)

Somewhat separately though: this adds a separate Java 9+ codepath. Is there any CI job that tests that? Scala 2 runs at least one JDK 17 CI run, but I don't see anything analogous in Dotty. I'm not sure that it is a good idea to merge code if it isn't ever going to be checked in CI...

@bishabosha
Copy link
Member

bishabosha commented Jul 27, 2021

Somewhat separately though: this adds a separate Java 9+ codepath. Is there any CI job that tests that? Scala 2 runs at least one JDK 17 CI run, but I don't see anything analogous in Dotty. I'm not sure that it is a good idea to merge code if it isn't ever going to be checked in CI...

by default the CI (e.g. Dotty / test) runs on JDK 16, then there is a separate Dotty / test_java8 for jdk 8 which is tested for releases

@anatoliykmetyuk
Copy link
Contributor

I've forgotten all of Dotty's conventions around testing

What conventions do you mean? If it helps, here's a docs page on how to run tests. Also, the workflow doc page may help. Tests themselves go in the tests directory, commonly into pos directory for tests expected to compile, run – for runtime tests, and neg for those that don't. T

@harpocrates
Copy link
Contributor Author

I've updated the test and it passes locally on my machine.

@harpocrates
Copy link
Contributor Author

Looking at the CI: is there a way to also run test_java8 for this PR just so we get some confidence the JDK 8 codepath is not broken?

@smarter
Copy link
Member

smarter commented Jul 28, 2021

If you edit the PR description (but not the PR title) to contain the string [test_java8] and then amend your commit and push-force, it should get run

@harpocrates
Copy link
Contributor Author

The scaladoc / stdlib-sourcelinks-test CI failure really does not look related to anything I did. The Java 8 run passed though, so I think this is ready to review.w

@smarter
Copy link
Member

smarter commented Jul 28, 2021

I think this is ready to review.

Great, however I won't be able to review this in the next two weeks, maybe @lrytz could have a look since he reviewed scala/scala#9556 ?

The scaladoc / stdlib-sourcelinks-test CI failure really does not look related to anything I did.

Yes this is a known issue: #13176

@griggt
Copy link
Contributor

griggt commented Jul 28, 2021

The scaladoc / stdlib-sourcelinks-test CI failure really does not look related to anything I did.

Yes this is a known issue: #13176

#13176 is actually a different problem. I don't think there is a ticket open for the sourcelinks CI failure, that workflow does not run during the nightly.

@griggt
Copy link
Contributor

griggt commented Jul 28, 2021

I don't think there is a ticket open for the sourcelinks CI failure

#13196

Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I guess we need to wait with merging for #13200 / #13196?

@smarter
Copy link
Member

smarter commented Jul 29, 2021

It's fine to ignore the failing job since we know it's unrelated to this PR.

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

Successfully merging this pull request may close these issues.

7 participants