Fix #4947: Retain the `structure` of `PreTransRecordTree`. #5091

sjrd · 2024-12-18T10:19:20Z

Instead of only retaining the RecordType.

The structure contains the complete information required to map FieldNames to the record's SimpleFieldNames.

Other than the newly added test, this PR causes 0 changes to the generated JavaScript code.

gzm0 · 2024-12-22T13:48:04Z

A higher level comment here, before I dive into the specifics: It seems there are two overall approaches to take to class field / record field mapping:

Dynamic: allocate record field names dynamically when we need them; track them. this is the status quo approach which was broken before and this PR fixes.
Static: fixed mapping between class field name / record field names (based on mangling of names, potentially even later, e.g. use field names in record types).

I was wondering if you have explored the static approach. It seems at least at face value, it would keep the optimizer simpler.

sjrd · 2024-12-22T14:06:14Z

It's a good question. I have thought a bit about it. I don't think there's a strong case either way.

One thing going for the dynamic approach is that it produces less code size in fastLinkJS. I am particularly concerned about expanded Longs, which already often "catch my eye" when I skim through test-suite/main.js.

Regarding complexity, I'm not sure the static approach would really be simpler. Overall, I find that keeping the structure is simpler, regardless of the static/dynamic approach. Or at least it's clearer to me. It is more obvious that it does the right thing too, especially when merging the results of If/Match/Labeled.

sjrd · 2024-12-22T16:08:59Z

Rebased to fix conflicts after #5092 was merged. No other changes.

gzm0

Or at least it's clearer to me. It is more obvious that it does the right thing too, especially when merging the results of If/Match/Labeled.

After review, I strongly agree with this. The joining points are much easier to understand with the approach in this PR than the approach before.

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

gzm0 · 2024-12-24T13:33:02Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

@@ -5660,7 +5671,10 @@ private[optimizer] object OptimizerCore {
  private val ClassTagApplyMethodName =
    MethodName("apply", List(ClassRef(ClassClass)), ClassRef(ClassName("scala.reflect.ClassTag")))

-  final class InlineableClassStructure(private val allFields: List[FieldDef]) {
+  final class InlineableClassStructure(val className: ClassName, private val allFields: List[FieldDef]) {
+    private[OptimizerCore] val refinedType: RefinedType =


What is this useful for? IIUC this is only used in tpe of PreTransRecordTree. And that usage feels very odd (I have commented there).

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

gzm0 · 2024-12-24T13:44:15Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

@@ -6286,9 +6309,13 @@ private[optimizer] object OptimizerCore {
   *  whereas `tree.tpe` is always the lowered `RecordType`.
   */


This specification of tpe feels odd. From the doc of PreTransform:

A PreTransform has a tpe as precisely refined as if a full transformExpr() had been performed.

So this is technically correct: a full transformExpr will roll-back any PreTransRecord tree (with the exception of RT long).

Do you know when this is useful in practice?

No, right now I'm not sure. I can check later. For now I've addressed all the other comments.

How would you like to proceed with this? It seems this is the last open comment on this PR.

From my second look, it seems like before this PR, we needed the tpe field to validate if two PreTransRecordTrees correspond to the same class. In hindsight, it is not much less obvious to me in the original code that the optimizer was correct in that regard, we're just moving form rhsOrigType != lhsOrigType to lhsStructure.sameClassAs(rhsStructure).

This PR introduces a clear distinction between RefinedTypes and (transient) allocated structures. I think this is an improvement we should keep; it makes many decisions the optimizer makes more explicit (notably in label processing this becomes clear).

However, this feels like a strong hint, that requiring a tpe: RefinedType on an arbitrary pre-transform might not be ideal. So maybe we should try an move it up the hierarchy?

I've had a look at usages of PreTransform#tpe and IMO it's fairly quickly clear why it is useful. For example, take IsInstanceOf:

scala-js/linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

Lines 558 to 576 in f82e59f

case IsInstanceOf(expr, testType) =>

trampoline {

pretransformExpr(expr) { texpr =>

val result = {

if (isSubtype(texpr.tpe.base.toNonNullable, testType)) {

if (texpr.tpe.isNullable)

BinaryOp(BinaryOp.!==, finishTransformExpr(texpr), Null())

else

Block(finishTransformStat(texpr), BooleanLiteral(true))

} else {

if (texpr.tpe.isExact)

Block(finishTransformStat(texpr), BooleanLiteral(false))

else

IsInstanceOf(finishTransformExpr(texpr), testType)

}

}

TailCalls.done(result)

}

}

PreTransform#tpe allows the transformation of IsInstanceOf to fold the operation if the type is statically know. We want this to happen, if the PreTransform ends up as a record.

Another usage I've found is to accurately type bindings: I suspect without the tpe, we'd have to roll-back the binding itself in case it needs var replacement (like this, we only need to roll-back the pretransform).

IMO this is sufficient to move forward with this PR, but PTAL as well.

Ah indeed, the IsInstanceOf example is pretty obvious. Another one where it is clearly the right thing is in pretansformApply to resolve targets and decide whether and what to inline:

scala-js/linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

Line 1966 in f82e59f

val className = boxedClassForType(treceiver.tpe.base)

Instead of only retaining the `RecordType`. The `structure` contains the complete information required to map `FieldName`s to the record's `SimpleFieldName`s.

sjrd

Updated. I haven't addressed the question about the spec of PreTransRecordTree.tpe.

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

sjrd · 2024-12-25T16:28:46Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

@@ -6286,9 +6309,13 @@ private[optimizer] object OptimizerCore {
   *  whereas `tree.tpe` is always the lowered `RecordType`.
   */


No, right now I'm not sure. I can check later. For now I've addressed all the other comments.

gzm0

Just the open comment about tpe. I might try and fiddle with it a bit today if I find the time.

gzm0 · 2024-12-27T09:14:38Z

linker/shared/src/main/scala/org/scalajs/linker/frontend/optimizer/OptimizerCore.scala

@@ -6286,9 +6309,13 @@ private[optimizer] object OptimizerCore {
   *  whereas `tree.tpe` is always the lowered `RecordType`.
   */


How would you like to proceed with this? It seems this is the last open comment on this PR.

From my second look, it seems like before this PR, we needed the tpe field to validate if two PreTransRecordTrees correspond to the same class. In hindsight, it is not much less obvious to me in the original code that the optimizer was correct in that regard, we're just moving form rhsOrigType != lhsOrigType to lhsStructure.sameClassAs(rhsStructure).

This PR introduces a clear distinction between RefinedTypes and (transient) allocated structures. I think this is an improvement we should keep; it makes many decisions the optimizer makes more explicit (notably in label processing this becomes clear).

However, this feels like a strong hint, that requiring a tpe: RefinedType on an arbitrary pre-transform might not be ideal. So maybe we should try an move it up the hierarchy?

sjrd requested a review from gzm0 December 18, 2024 10:19

sjrd force-pushed the fix-record-simple-names branch from b76258f to 540550e Compare December 22, 2024 16:06

gzm0 requested changes Dec 24, 2024

View reviewed changes

Fix scala-js#4947: Retain the structure of PreTransRecordTree.

24c79c2

Instead of only retaining the `RecordType`. The `structure` contains the complete information required to map `FieldName`s to the record's `SimpleFieldName`s.

sjrd force-pushed the fix-record-simple-names branch from 540550e to 24c79c2 Compare December 25, 2024 16:29

sjrd commented Dec 25, 2024

View reviewed changes

gzm0 requested changes Dec 27, 2024

View reviewed changes

gzm0 approved these changes Dec 27, 2024

View reviewed changes

sjrd merged commit 7da555a into scala-js:main Dec 27, 2024
3 checks passed

sjrd deleted the fix-record-simple-names branch December 27, 2024 10:40

sjrd mentioned this pull request Jan 5, 2025

Remove dead code in OptimizerCore. #5103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4947: Retain the `structure` of `PreTransRecordTree`. #5091

Fix #4947: Retain the `structure` of `PreTransRecordTree`. #5091

sjrd commented Dec 18, 2024

gzm0 commented Dec 22, 2024

sjrd commented Dec 22, 2024

sjrd commented Dec 22, 2024

gzm0 left a comment

gzm0 Dec 24, 2024

gzm0 Dec 24, 2024

sjrd Dec 25, 2024

gzm0 Dec 27, 2024

gzm0 Dec 27, 2024

sjrd Dec 27, 2024

sjrd left a comment

sjrd Dec 25, 2024

gzm0 left a comment

gzm0 Dec 27, 2024

		@@ -6286,9 +6309,13 @@ private[optimizer] object OptimizerCore {
		* whereas `tree.tpe` is always the lowered `RecordType`.
		*/

	case IsInstanceOf(expr, testType) =>
	trampoline {
	pretransformExpr(expr) { texpr =>
	val result = {
	if (isSubtype(texpr.tpe.base.toNonNullable, testType)) {
	if (texpr.tpe.isNullable)
	BinaryOp(BinaryOp.!==, finishTransformExpr(texpr), Null())
	else
	Block(finishTransformStat(texpr), BooleanLiteral(true))
	} else {
	if (texpr.tpe.isExact)
	Block(finishTransformStat(texpr), BooleanLiteral(false))
	else
	IsInstanceOf(finishTransformExpr(texpr), testType)
	}
	}
	TailCalls.done(result)
	}
	}

Fix #4947: Retain the structure of PreTransRecordTree. #5091

Fix #4947: Retain the structure of PreTransRecordTree. #5091

Conversation

sjrd commented Dec 18, 2024

gzm0 commented Dec 22, 2024

sjrd commented Dec 22, 2024

sjrd commented Dec 22, 2024

gzm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix #4947: Retain the `structure` of `PreTransRecordTree`. #5091

Fix #4947: Retain the `structure` of `PreTransRecordTree`. #5091