Fix #2185: Add bytecode idempotency checks. #2272

nicolasstucki · 2017-04-18T13:11:38Z

Add bytecodeIdemporency to compilation tests. This tests compiles any compilation test twice and compares the resulting bytecodes (currently dotty-compiler and tests/pos).

DarkDimius · 2017-04-18T13:20:00Z

compiler/src/dotty/tools/dotc/core/tasty/TastyPickler.scala

@@ -14,7 +14,7 @@ import Decorators._
 class TastyPickler {

  private val sections = new mutable.ArrayBuffer[(NameRef, TastyBuffer)]
-  val uuid = UUID.randomUUID()
+  private val uuid = UUID.fromString("3cee1b79-c03a-4125-b337-d067b5cb3a94") // TODO: use a hash of the tasty tree


I'd prefer not to merge this change as-is.

@DarkDimius, you talked about making the hash for it. Did you have something specific in mind?

@DarkDimius I changes the tasty UUID to a hash of the nameBuffer and sections.

This idea looks good to me.

smarter · 2017-04-18T16:21:17Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

+    val blacklisted = Set(
+      "pos/Map/scala/collection/immutable/Map",
+      "pos/Map/scala/collection/immutable/AbstractMap",
+      "pos/t1203a/NodeSeq"


Why is there a blacklist? If these files fail the test, shoudn't issues be filed for them?

There will. I just haven't had time to do it yet.

Added them to #2274.

smarter · 2017-04-18T16:37:46Z

compiler/test/dotty/tools/dotc/CompilationTests.scala

+      compileFilesInDir("../tests/pos", opt)
+    }
+    def idempotency2() = {
+      compileList("dotty1", compilerSources ++ backendSources ++ backendJvmSources, opt) +


It would be interesting to combine this with a bootstrap test: Compile the compiler, then compile the compiler with the compiled compiler, and check equality.

That will be interesting, but I would add it as a second step in another PR.

DarkDimius

LGTM.

DarkDimius · 2017-04-26T09:17:01Z

compiler/src/dotty/tools/dotc/util/MurmurLongHash3.scala

+  * @see [[http://code.google.com/p/smhasher]]
+  */
+object MurmurLongHash3 extends MurmurLongHash3 {
+  final val arraySeed       = 0x3c074a61


Why are seeds ints?

smarter · 2017-04-26T09:58:21Z

compiler/src/dotty/tools/dotc/util/MurmurLongHash3.scala

+  * objects.
+  *
+  * This algorithm is designed to generate well-distributed non-cryptographic
+  * hashes. It is designed to hash data in 32 bit chunks (ints).


64 now, right ?

smarter · 2017-04-26T10:03:42Z

compiler/src/dotty/tools/dotc/util/MurmurLongHash3.scala

+
+import java.lang.Long.{ rotateLeft => rotl }
+
+private[util] class MurmurLongHash3 {


Nice! I think this should be contributed upstream to scala-library.

Good idea. But I need to reimplement the algorithm from the source. @DarkDimius showed me some issues with my simple port. I will try to make it compatible MurmurHash3 to be able to just add it inside in the future.

DarkDimius · 2017-04-26T13:15:57Z

As discussed in person, using murmur3 to emmit 64-bit hashes is not evidenced to be a good hash.
Similarly, using 128bit mumur3 and taking part of is isn't good iether(murmu3 is not cryptographic hash).

I propose we don't use murmur family for this one, and instead use FarmHash / CityHash, which have been open-sourced by google and are used to hash entries in maps(our use-case).

smarter · 2017-04-26T13:42:55Z

As discussed in person, using murmur3 to emmit 64-bit hashes is not evidenced to be a good hash.

Interesting, why?

nicolasstucki · 2017-04-28T09:06:23Z

Updated hashing

smarter · 2017-04-28T09:08:44Z

compiler/src/dotty/tools/dotc/util/CityHash.scala

@@ -0,0 +1,186 @@
+package dotty.tools.dotc.util
+
+/* Ported from https://github.com/google/cityhash/blob/master/src/city.cc */


You should preserve the copyright/license header of the original file, and you should update AUTHORS.md to note this.

smarter · 2017-04-28T09:11:12Z

compiler/src/dotty/tools/dotc/util/CityHash.scala

+
+  private final def fetch64(idx: Int)(implicit data: Array[Byte]): Long = {
+    var x: Long = data(idx)
+    x = data(idx + 1) | (x << 8)


Careful with sign extension (because Byte is signed). If I recall correctly, you need to add& 0xff everywhere you convert from a Byte to an Int to avoid that.

You should add some unit tests for this class, to make sure it behaves like the reference implementation.

nicolasstucki · 2017-05-04T08:33:05Z

@DarkDimius the hash function was updated.

DarkDimius · 2017-05-04T09:48:25Z

As discussed in person, using murmur3 to emmit 64-bit hashes is not evidenced to be a good hash.

Interesting, why?

While real cryptographic hashes are expected to remain "good" if you take a subsection of it, you don't have this guarantee for non-cryptograhic hashes in general.
Murmur3 family is not cryptographic and does not include 64-bit hash. Using 128-bit hash and only taking a part of it may be a weak hash and I didn't quickly find an evidence to be proven otherwise.

That's why I've proposed FarmHash / CityHash that have native 64-bit variants.

@nicolasstucki, I've lost context, we did we decide to use Elf hashing instead of CityHash?

nicolasstucki · 2017-05-04T13:19:04Z

The context for using Elf hashing instead of CityHash was because the implementation is trivial and it is simpler to be sure that it is correctly implemented. But, if we want to use CityHash we can just revert 426a33a.

felixmulder · 2017-05-05T08:28:44Z

@DarkDimius - is this good to go now?

DarkDimius reviewed Apr 18, 2017

View reviewed changes

smarter reviewed Apr 18, 2017

View reviewed changes

smarter mentioned this pull request Apr 19, 2017

tests/pos/Map.scala does not produce idempotent bytecode #2274

Closed

DarkDimius approved these changes Apr 19, 2017

View reviewed changes

nicolasstucki force-pushed the idempotency-checks branch 2 times, most recently from 5b1dd94 to 6e3b580 Compare April 19, 2017 15:14

DarkDimius reviewed Apr 26, 2017

View reviewed changes

smarter reviewed Apr 26, 2017

View reviewed changes

nicolasstucki added the stat:do not merge label Apr 26, 2017

nicolasstucki force-pushed the idempotency-checks branch 2 times, most recently from 37ae02d to 9f57381 Compare April 27, 2017 12:06

nicolasstucki added stat:needs review and removed stat:do not merge labels Apr 28, 2017

smarter reviewed Apr 28, 2017

View reviewed changes

nicolasstucki added 6 commits May 3, 2017 11:27

Fix scala#2185: Add bytecode idempotency checks.

5d29e7e

Compute tasty UUID based on nameBuffer and sections hash.

1dd46f2

Use mumur hashing for tasty uuid.

3caa001

Port and use 64-bit CityHash for tasty uuid.

54560a6

Add copyright

5cbd788

Fix byte to long casts.

c73fd4b

nicolasstucki force-pushed the idempotency-checks branch from 9f57381 to eb4be1f Compare May 3, 2017 11:22

nicolasstucki added 2 commits May 4, 2017 15:44

Add test to idempotency blacklist.

eed4b9b

Implement TastyHash based on PJW hash.

000b432

nicolasstucki force-pushed the idempotency-checks branch from eb4be1f to 000b432 Compare May 4, 2017 13:48

DarkDimius merged commit f28c984 into scala:master May 5, 2017

allanrenucci deleted the idempotency-checks branch December 14, 2017 19:23

Blaisorblade mentioned this pull request Aug 6, 2018

Fix tasty hash #4865

Merged


		import java.lang.Long.{ rotateLeft => rotl }

		private[util] class MurmurLongHash3 {

		@@ -0,0 +1,186 @@
		package dotty.tools.dotc.util

		/* Ported from https://github.com/google/cityhash/blob/master/src/city.cc */

Fix #2185: Add bytecode idempotency checks. #2272

Fix #2185: Add bytecode idempotency checks. #2272

Uh oh!

Conversation

nicolasstucki commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkDimius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarkDimius commented Apr 26, 2017

Uh oh!

smarter commented Apr 26, 2017

Uh oh!

nicolasstucki commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented May 4, 2017

Uh oh!

DarkDimius commented May 4, 2017

Uh oh!

nicolasstucki commented May 4, 2017

Uh oh!

felixmulder commented May 5, 2017

Uh oh!

Uh oh!

nicolasstucki Apr 19, 2017 •

edited

Loading