Skip to content

update BackendUtils.classfileVersionMap#23954

Merged
tanishiking merged 1 commit intoscala:mainfrom
xuwei-k:classfileVersionMap
Dec 26, 2025
Merged

update BackendUtils.classfileVersionMap#23954
tanishiking merged 1 commit intoscala:mainfrom
xuwei-k:classfileVersionMap

Conversation

@xuwei-k
Copy link
Copy Markdown
Contributor

@xuwei-k xuwei-k commented Sep 18, 2025 •
edited
Loading

@xuwei-k xuwei-k force-pushed the classfileVersionMap branch from ba0b8c3 to dd87778 Compare September 18, 2025 13:03
He-Pin
He-Pin reviewed Sep 18, 2025
View reviewed changes
Comment thread compiler/src/dotty/tools/backend/jvm/BackendUtils.scala
14 -> asm.Opcodes.V14,
15 -> asm.Opcodes.V15,
16 -> asm.Opcodes.V16,
17 -> asm.Opcodes.V17,
Copy link
Copy Markdown
Contributor

@He-Pin He-Pin Sep 18, 2025

Choose a reason for hiding this comment

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

Add a comment here:

//java 8~16 support has been dropped`

@xuwei-k

This comment was marked as outdated.

Sign in to view
@xuwei-k

This comment was marked as outdated.

Sign in to view
@xuwei-k xuwei-k closed this Sep 18, 2025
@xuwei-k xuwei-k deleted the classfileVersionMap branch September 21, 2025 11:41
@xuwei-k xuwei-k restored the classfileVersionMap branch November 28, 2025 12:06
@xuwei-k xuwei-k reopened this Nov 28, 2025
@xuwei-k xuwei-k force-pushed the classfileVersionMap branch 4 times, most recently from 54a2f0c to eee5ebf Compare December 1, 2025 00:24
@xuwei-k xuwei-k marked this pull request as ready for review December 1, 2025 02:06
@Gedochao Gedochao added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Dec 22, 2025
@Gedochao Gedochao added this to the 3.8.0 milestone Dec 22, 2025
@Gedochao Gedochao requested a review from noti0na1 December 22, 2025 07:50
WojciechMazur
WojciechMazur approved these changes Dec 22, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@WojciechMazur WojciechMazur left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for these changes.

Potentially we can also remove the now deadcode logic for String Concat used in JDK8

scala3/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala

Lines 1306 to 1326 in 6bdf209

// `StringConcatFactory` only got added in JDK 9, so use `StringBuilder` for lower
if (backendUtils.classfileVersion < asm.Opcodes.V9) {
// Estimate capacity needed for the string builder
val approxBuilderSize = concatArguments.view.map {
case Literal(Constant(s: String)) => s.length
case Literal(c @ Constant(_)) if c.isNonUnitAnyVal => String.valueOf(c).length
case _ => 0
}.sum
bc.genNewStringBuilder(approxBuilderSize)
stack.push(jlStringBuilderRef) // during the genLoad below, there is a reference to the StringBuilder on the stack
for (elem <- concatArguments) {
val elemType = tpeTK(elem)
genLoad(elem, elemType)
bc.genStringBuilderAppend(elemType)
}
stack.pop()
bc.genStringBuilderEnd
} else {

However it can also be done in the follow up. I leave the decision up to you, we'd most likely merge it tomorrow and backport to 3.8.0-RC5

tanishiking
tanishiking requested changes Dec 22, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@tanishiking tanishiking left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One nitpick, but otherwise LGTM from myside. (but we can merge it without fixing if we're in rush)

Comment thread compiler/test/dotty/tools/backend/jvm/StringConcatTest.scala
"append(D)Ljava/lang/StringBuilder;",
"append(Ljava/lang/StringBuffer;)Ljava/lang/StringBuilder;",
"append(Ljava/lang/CharSequence;)Ljava/lang/StringBuilder;",
"append(Ljava/lang/Object;)Ljava/lang/StringBuilder;", // test that we're not using the [C overload
Copy link
Copy Markdown
Member

@tanishiking tanishiking Dec 22, 2025

Choose a reason for hiding this comment

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

because Java9+ uses https://openjdk.org/jeps/280 👍

Comment thread compiler/test/dotty/tools/backend/jvm/StringInterpolatorOptTest.scala Outdated
class StringInterpolatorOptTest extends DottyBytecodeTest {
import ASMConverters._

private def convertInvokeDynamicArray(i: Instruction): Instruction =
Copy link
Copy Markdown
Member

@tanishiking tanishiking Dec 22, 2025

Choose a reason for hiding this comment

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

I believe this is converting arrays to lists for structural equality of InvokeDynamic, but bsmArgs shouldn't have Array...
It turned out this is the consequence of a long-time bug in AsmConverters 😄 The following diff should fix without convertInvokeDynamicArray.

diff --git a/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala b/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala
index c751937bd9..dfa7ed25ab 100644
--- a/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala
+++ b/compiler/test/dotty/tools/backend/jvm/AsmConverters.scala
@@ -148,7 +148,7 @@ object ASMConverters {
 
     private def convertBsmArgs(a: Array[Object]): List[Object] = a.iterator.map({
       case h: asm.Handle => convertMethodHandle(h)
-      case _ => a // can be: Class, method Type, primitive constant
+      case x => x // can be: Class, method Type, primitive constant
     }).toList
 
     private def convertMethodHandle(h: asm.Handle): MethodHandle = MethodHandle(h.getTag, h.getOwner, h.getName, h.getDesc, h.isInterface)

Copy link
Copy Markdown
Contributor Author

@xuwei-k xuwei-k Dec 26, 2025

Choose a reason for hiding this comment

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

Thanks!

@xuwei-k xuwei-k force-pushed the classfileVersionMap branch from eee5ebf to 2b92b26 Compare December 26, 2025 04:07
@xuwei-k xuwei-k force-pushed the classfileVersionMap branch from 2b92b26 to 042bd3f Compare December 26, 2025 04:09
tanishiking
tanishiking approved these changes Dec 26, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@tanishiking tanishiking left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tanishiking tanishiking merged commit 82acfa7 into scala:main Dec 26, 2025
95 of 96 checks passed
@xuwei-k xuwei-k deleted the classfileVersionMap branch December 26, 2025 07:10
@xuwei-k xuwei-k mentioned this pull request Dec 26, 2025
Merged
@xuwei-k
Copy link
Copy Markdown
Contributor Author

xuwei-k commented Dec 26, 2025

Potentially we can also remove the now deadcode logic for String Concat used in JDK8

WojciechMazur pushed a commit that referenced this pull request Dec 26, 2025
@xuwei-k
Remove unnecessary code in BCode (#24832)
0dc10e0 
- #23954
@WojciechMazur WojciechMazur mentioned this pull request Dec 30, 2025
Merged
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Dec 30, 2025
WojciechMazur added a commit that referenced this pull request Dec 31, 2025
@WojciechMazur
Backport "update BackendUtils.classfileVersionMap" to 3.8.0 (#24864)
b5074c9 
Backports #23954 to the 3.8.0-RC5.

PR submitted by the release tooling.
[skip ci]
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Dec 31, 2025
@WojciechMazur WojciechMazur mentioned this pull request Jan 13, 2026
Merged
WojciechMazur added a commit that referenced this pull request Jan 13, 2026
@WojciechMazur
Backport "update BackendUtils.classfileVersionMap" to 3.8.1 (#24958)
d792dd6 
Backports #23954 to the 3.8.1-RC1.

PR submitted by the release tooling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tanishiking tanishiking tanishiking approved these changes

@WojciechMazur WojciechMazur WojciechMazur approved these changes

@noti0na1 noti0na1 Awaiting requested review from noti0na1

+1 more reviewer

@He-Pin He-Pin He-Pin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@tanishiking tanishiking

Labels

backport:done This PR was successfully backported.

Projects

None yet

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

use StringConcatFactory by default

6 participants

@xuwei-k @He-Pin @tanishiking @WojciechMazur @Kordyjan @Gedochao