Unify TreeHash and Versioned into a single Version #4772

gzm0 · 2022-12-23T08:56:01Z

No description provided.

gzm0 · 2022-12-23T08:56:11Z

Only the last commit is part of this PR.

sjrd

Nice. I like it :)

sjrd · 2022-12-23T09:36:04Z

ir/shared/src/main/scala/org/scalajs/ir/Serializers.scala

+      val isHash = version.isHash
+      buffer.writeBoolean(isHash)
+      if (isHash)
+        version.writeHash(buffer)


What happens/should happen if the version is Combined here?

Do not write it. The idea (and yes I'll document this properly) is that we'll only ever serialize hashed versions. All the other versions are not serialization safe (as in: we cannot guarantee collision freedom across linker instances).

sjrd · 2022-12-23T09:43:02Z

linker/jvm/src/main/scala/org/scalajs/linker/PathIRFile.scala

+  private[linker] def fileTimeToVersion(time: FileTime): ir.Version =
+    ir.Version.fromBytes(time.toString().getBytes(StandardCharsets.US_ASCII))


That seems unnecessarily inefficient. Consider something like:

val instant = time.toInstant() ir.Version.fromLongs(instant.getEpochSecond(), instant.getNano())

I have. But that isn't lossless according to this: https://docs.oracle.com/javase/8/docs/api/java/nio/file/attribute/FileTime.html#toInstant--

toString is the only lossless serialization of FileTime I could find.

Hum, OK. Probably worth a comment, then.

sjrd · 2022-12-23T09:48:05Z

linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/Emitter.scala

        } { linkedInit =>
-          mergeVersions(linkedClass.version, linkedInit.version).map("2-" + _)
+          Version.combine(linkedClass.version, linkedInit.version)


This is a bit funny. We can distinguish those two Versions because Combine of different lengths are always different. That's a bit of an implementation detail, though, isn't it? Also, what if we wanted the same distinguishing feature but coming from two different single Versions?

Should we have a Version.tag(tag: Int, underlying: Version) constructor for that purpose?

I have considered that. IMO that is subsumed by:

Version.combine(Version.fromInt(tag), underlying)

(arguably that is slighly less space efficient, but I'm not sure it matters).

Ah yes, that makes sense.

gzm0 · 2022-12-23T11:06:54Z

project/JavalibIRCleaner.scala

@@ -204,7 +204,7 @@ final class JavalibIRCleaner(baseDirectoryURI: URI) {
        case m @ MethodDef(flags, name, originalName, args, resultType, body) =>
          implicit val pos = m.pos
          MethodDef(flags, transformMethodIdent(name), originalName, transformParamDefs(args),
-              transformType(resultType), body)(m.optimizerHints, m.hash)
+              transformType(resultType), body)(m.optimizerHints, Version.NoVersion)


Note: We're running the overall result through Hashers anyways (as we should). So this will just keep an old / wrong hash (in fact, it could lead to spurious diffs where there are non in the resulting tree).

gzm0 · 2022-12-23T18:50:03Z

Not ready for review yet. Needs test adjustment (and tests).

gzm0 · 2022-12-24T12:43:32Z

Ready for review now.

sjrd

Minor comments only.

sjrd · 2022-12-24T15:53:10Z

ir/shared/src/main/scala/org/scalajs/ir/Version.scala

+final class Version private (private val v: Array[Byte]) extends AnyVal {
+  import Version.Type
+
+  /** Whether this is a sentinel. */


A sentinel is an implementation-detail concept. I wouldn't talk about it in the public API. Also this suggests that it returns true when it's the sentinel.

Consider instead "Whether this version is different than NoVersion." or something like that.

Seems like we weren't actually using this a public API, so I made it private and dropped the comment entirely.

sjrd · 2022-12-24T16:03:40Z

ir/shared/src/test/scala/org/scalajs/ir/VersionTest.scala

+
+    testNe(v, fromHash(a))
+    testNe(v, fromBytes(a))
+


Spurious new line.

sjrd · 2022-12-24T16:14:06Z

project/BinaryIncompatibilities.scala

-    exclude[DirectMissingMethodProblem]("org.scalajs.ir.Trees#JSPropertyDef.apply"),
-    exclude[MissingClassProblem]("org.scalajs.ir.Trees$DoWhile"),
-    exclude[MissingClassProblem]("org.scalajs.ir.Trees$DoWhile$"),
+    exclude[Problem]("org.scalajs.ir.*"),


sjrd · 2022-12-24T16:17:25Z

Have you considered calling NoVersion instead Unknown? It might make it clearer why Unknown is not the same version as Unknown.

gzm0 · 2022-12-25T08:01:52Z

Have you considered calling NoVersion instead Unknown? It might make it clearer why Unknown is not the same version as Unknown.

Good point. Unversioned also comes to mind (I feel this might almost be a bit better). I don't have a strong opinion on this, so any is fine by me.

sjrd · 2022-12-25T14:54:18Z

No real preference between Unkown and Unversioned.

gzm0 · 2022-12-25T15:41:27Z

Updated.

sjrd reviewed Dec 23, 2022

View reviewed changes

gzm0 force-pushed the version branch 3 times, most recently from b41a68b to 88f6988 Compare December 23, 2022 11:04

gzm0 changed the title ~~RFC: Use a unified version type for serialization and linking~~ Unify TreeHash and Versioned into a single Version Dec 23, 2022

gzm0 commented Dec 23, 2022

View reviewed changes

gzm0 force-pushed the version branch from 88f6988 to 185711c Compare December 23, 2022 15:21

gzm0 requested review from sjrd and removed request for sjrd December 23, 2022 15:21

gzm0 force-pushed the version branch 3 times, most recently from e130295 to ebe5066 Compare December 24, 2022 12:42

gzm0 requested a review from sjrd December 24, 2022 12:43

sjrd requested changes Dec 24, 2022

View reviewed changes

gzm0 marked this pull request as ready for review December 25, 2022 08:00

gzm0 force-pushed the version branch from ebe5066 to 3855b1d Compare December 25, 2022 08:00

gzm0 requested a review from sjrd December 25, 2022 08:02

gzm0 force-pushed the version branch from 3855b1d to ad3e477 Compare December 25, 2022 15:41

sjrd approved these changes Dec 25, 2022

View reviewed changes

gzm0 force-pushed the version branch 3 times, most recently from eb5773c to 321e993 Compare December 26, 2022 11:19

Unify TreeHash and Versioned into a single Version

9f5071f

gzm0 force-pushed the version branch from 321e993 to 9f5071f Compare December 26, 2022 14:44

gzm0 merged commit 205b6d7 into scala-js:main Dec 26, 2022

gzm0 deleted the version branch December 26, 2022 17:35

gzm0 mentioned this pull request Jan 3, 2025

Add a desugaring step in the base linker. #5096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify TreeHash and Versioned into a single Version #4772

Unify TreeHash and Versioned into a single Version #4772

gzm0 commented Dec 23, 2022

gzm0 commented Dec 23, 2022

sjrd left a comment

sjrd Dec 23, 2022

gzm0 Dec 23, 2022

sjrd Dec 23, 2022

gzm0 Dec 23, 2022

sjrd Dec 23, 2022

sjrd Dec 23, 2022

gzm0 Dec 23, 2022

sjrd Dec 23, 2022

gzm0 Dec 23, 2022

gzm0 commented Dec 23, 2022

gzm0 commented Dec 24, 2022

sjrd left a comment

sjrd Dec 24, 2022

gzm0 Dec 25, 2022

sjrd Dec 24, 2022

sjrd Dec 24, 2022

sjrd commented Dec 24, 2022

gzm0 commented Dec 25, 2022

sjrd commented Dec 25, 2022

gzm0 commented Dec 25, 2022

		private[linker] def fileTimeToVersion(time: FileTime): ir.Version =
		ir.Version.fromBytes(time.toString().getBytes(StandardCharsets.US_ASCII))

Unify TreeHash and Versioned into a single Version #4772

Unify TreeHash and Versioned into a single Version #4772

Conversation

gzm0 commented Dec 23, 2022

gzm0 commented Dec 23, 2022

sjrd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 commented Dec 23, 2022

gzm0 commented Dec 24, 2022

sjrd 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

sjrd commented Dec 24, 2022

gzm0 commented Dec 25, 2022

sjrd commented Dec 25, 2022

gzm0 commented Dec 25, 2022