Optimize away ClassTags and ArrayBuilders #1669

sjrd · 2015-05-16T20:13:35Z

No description provided.

sjrd · 2015-05-16T20:13:41Z

~~CI for now.~~

scala-jenkins · 2015-05-16T22:13:28Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1698/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2201/
Test PASSed.

scala-jenkins · 2015-05-16T23:27:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1699/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2202/
Test FAILed.

scala-jenkins · 2015-05-17T05:26:56Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1702/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2205/
Test PASSed.

scala-jenkins · 2015-05-17T08:51:05Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1703/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2207/
Test PASSed.

sjrd · 2015-05-17T12:39:35Z

OK, I'm done with this one.
Review by @gzm0 please.

scala-jenkins · 2015-05-17T12:59:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1705/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2209/
Test PASSed.

scala-jenkins · 2015-05-17T14:13:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1704/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2208/
Test PASSed.

scala-jenkins · 2015-05-17T16:08:01Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1706/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2210/
Test PASSed.

gzm0 · 2015-05-17T19:10:36Z

What is Intf?

gzm0 · 2015-05-17T19:11:33Z

ir/src/main/scala/org/scalajs/core/ir/Types.scala

-              isSubclass(lhsBase, rhsBase)
+              /* All things must be considered subclasses of Object for this
+               * purpose, even raw JS types and interfaces that do not have
+               * Object in their ancestors, strictly speaking.


What do you mean by "strictly speaking"?

Hum, nothing, really. I guess when I wrote that I was still surprised myself to rediscover that interfaces did not have Object in their ancestors.

gzm0 · 2015-05-17T19:14:05Z

Also, why is not every class type subtype of Object anyways?

gzm0 · 2015-05-17T19:16:40Z

Why not generalize Select elimination to any target?

gzm0 · 2015-05-17T19:24:29Z

test-suite/src/test/scala/org/scalajs/testsuite/javalib/ReflectArray.scala

@@ -0,0 +1,72 @@
+/*                     __                                               *\
+**     ________ ___   / /  ___      __ ____  Scala.js Test Suite        **


This file should be ReflectArrayTest.scala.

sjrd · 2015-05-17T19:29:12Z

What is Intf?

Short for Interface. Isn't that the standard shorthand?

gzm0 · 2015-05-17T19:31:54Z

Short for Interface. Isn't that the standard shorthand?

Hahahaha, I have no idea. But I though its something like Int and Float :P Let's have google decide.

sjrd · 2015-05-17T19:33:55Z

Also, why is not every class type subtype of Object anyways?

For raw JS types, because they're not. They're subtypes of Any, but not of Object, which is only supertype of all Scala.js classes + hijacked classes.

For interfaces, well, it might be an accident of history, because interfaces in .class files do not list Object as their superclass. So sjsir files do not either. But I guess we could instead decide that Object is part of the ancestors of interfaces once in a LinkedClass. That might make more sense.

gzm0 · 2015-05-17T19:35:25Z

Intf vs Iface:

"intf java": 89k
"iface java": 238k
"inft scala": 6k
"iface scala": 587k

sjrd · 2015-05-17T19:35:28Z

Why not generalize Select elimination to any target?

Because the qualifier could be null. Although, arguably, the optimizer already ignores that possibility in some cases (when inlining a method, for example). So yes, we could.

gzm0 · 2015-05-17T19:36:16Z

Because the qualifier could be null. Although, arguably, the optimizer already ignores that possibility in some cases (when inlining a method, for example). So yes, we could.

Seems like we need a semantic flag :)

gzm0 · 2015-05-17T19:38:17Z

For raw JS types, because they're not. They're subtypes of Any, but not of Object, which is only supertype of all Scala.js classes + hijacked classes.

Hmmm... nevertheless you can call all methods of Object on a raw JS type...

gzm0 · 2015-05-17T19:39:38Z

For interfaces, well, it might be an accident of history, because interfaces in .class files do not list Object as their superclass. So sjsir files do not either. But I guess we could instead decide that Object is part of the ancestors of interfaces once in a LinkedClass. That might make more sense.

I think we should do that. Doesn't this also apply in other (non-array) cases? We know that any non raw interface is a subtype of Object right?

sjrd · 2015-05-17T19:40:47Z

Seems like we need a semantic flag :)

Even without optimizer, we never truly respect NPEs, and we never will, even under a flag, because

NPEs are TypeErrors in the first place
In a method call, the time at which a TypeError is thrown (before evaluating the arguments) is different than that at which an NPE is thrown (after evaluating the arguments).

Checking things accurately would be so awful, from both a performance perspective than a maintenance of JSDesaguring perspective, that this will never happen.

gzm0 · 2015-05-17T19:42:31Z

Oh. OK for NPEs. If they are officially undefined behavior, we can do whatever we want :) So let's elide Select all the time.

gzm0 · 2015-05-17T20:02:43Z

scalalib/overrides/scala/collection/mutable/ArrayBuilder.scala

+      genericArrayBuilderResult(
+          if (classOf[Unit] == elementClass) classOf[BoxedUnit]
+          else if (classOf[Null] == elementClass || classOf[Nothing] == elementClass) classOf[Object]
+          else elementClass,


Consider making a tmp var with this (except if it hinders inlining). It is hard to read.

It does not do harm to inlining, no. It just produces a bit more JS code in the case where the runtimeClass is not known statically.
(edit: so I'll change it)

gzm0 · 2015-05-17T20:08:14Z

I'll leave that change out of this PR, anyway.

OK.

I'll continue this later. The Array inlining tests were too much :)

gzm0 · 2015-05-17T20:10:25Z

By the way, I think this PR is a good candidate for running a partest on it.

sjrd · 2015-05-17T20:12:09Z

By the way, I think this PR is a good candidate for running a partest on it.

Already did locally (that's how I found out that classOf[Null] and classOf[Nothing] had to be replaced by classOf[Object]). I'll schedule a nightly on the CI when the PR otherwise LGTY.

gzm0 · 2015-05-18T00:15:54Z

tools/shared/src/main/scala/org/scalajs/core/tools/optimizer/OptimizerCore.scala

+        }
+        cont(PreTransTree(CallHelper("makeNativeArrayWrapper",
+            CallHelper("arrayDataOf",
+                CallHelper("classDataOf", runtimeClass)(AnyType))(AnyType),


I don't like this since it exposes the existence of class data to the optimizer. Could we make a single dummy that takes a j.l.Class and a js.Array and produces the wrapped array?

Even the library knows that class data exist.

Well yes... but the bracket part of it... Let's leave it. Probably not doable differently.

gzm0 · 2015-05-18T00:38:03Z

That's all.

…ct]) is true.

The side effects of `qualifier` are kept, obviously. We can do this because NPE is undefined behavior.

LoadModule, NewArray, ArrayValue, GetClass and ClassOf always return non-nullable values of an exact type. This can help some other optimizations, the most obvious being optimizing away a test LoadModule(_) === Null().

sjrd · 2015-05-18T11:53:28Z

Updated.

* GetClass(expr) when expr has an exact type * clazz.getComponentType() * j.l.reflect.Array.newInstance(clazz, length) * Inline ScalaRunTime.{arrayClass,arrayElementClass}

This allows inlining of `Array.tabulate(x, y)`, for example, which is implemented as def tabulate[T: ClassTag](n1: Int, n2: Int)( f: (Int, Int) => T): Array[Array[T]] = tabulate(n1)(i1 => tabulate(n2)(f(i1, _))) in which the tabulate_1 method is called with a lambda that itself calls tabulate_1.

scala-jenkins · 2015-05-18T16:19:47Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):

https://scala-webapps.epfl.ch/jenkins/job/scalajs-pr/1709/
https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2214/
Test PASSed.

sjrd · 2015-05-18T17:42:16Z

Nightly: https://scala-webapps.epfl.ch/jenkins/job/scalajs-matrix-build/2216/

sjrd · 2015-05-19T08:00:59Z

Nightly passed on a85b6e4

gzm0 · 2015-05-20T18:47:42Z

LGTM

…builders Optimize away ClassTags and ArrayBuilders

sjrd force-pushed the optimize-away-classtags-and-array-builders branch from c41293a to 1c99868 Compare May 16, 2015 23:27

sjrd force-pushed the optimize-away-classtags-and-array-builders branch from 1c99868 to d229654 Compare May 17, 2015 06:50

sjrd force-pushed the optimize-away-classtags-and-array-builders branch 2 times, most recently from 9c5926a to 0969fe2 Compare May 17, 2015 12:24

sjrd changed the title ~~DO NOT MERGE Optimize away ClassTags and ArrayBuilders~~ Optimize away ClassTags and ArrayBuilders May 17, 2015

sjrd assigned gzm0 May 17, 2015

gzm0 reviewed May 17, 2015
View reviewed changes

gzm0 reviewed May 18, 2015
View reviewed changes

sjrd added 4 commits May 18, 2015 13:00

Fix scala-js#1668: isSubType(Array[Interface | RawJSType], Array[Obje…

3b00ab8

…ct]) is true.

Fix scala-js#1667: Fix getAncestorsOf for interfaces and raw JS types.

1222d58

Elide Select(qualifier, _) in statement position.

0d2869d

The side effects of `qualifier` are kept, obviously. We can do this because NPE is undefined behavior.

sjrd force-pushed the optimize-away-classtags-and-array-builders branch 2 times, most recently from 8c738ff to a85b6e4 Compare May 18, 2015 11:52

sjrd added 6 commits May 18, 2015 13:53

Constant-fold some operations on j.l.Class values.

8b9f01b

* GetClass(expr) when expr has an exact type * clazz.getComponentType() * j.l.reflect.Array.newInstance(clazz, length) * Inline ScalaRunTime.{arrayClass,arrayElementClass}

Add scala.reflect.{ClassTag,Manifest} to overrides to track changes.

06810dc

Make ClassTags inlineable and optimizable.

7865a5d

Add Array and ArrayBuilder to overrides to track changes.

ccbfb63

An optimized ArrayBuilder for Scala.js.

58c6f31

sjrd mentioned this pull request May 20, 2015

Fix #1653: Implement java.lang.Class.cast(). #1657

Merged

gzm0 added a commit that referenced this pull request May 20, 2015

Merge pull request #1669 from sjrd/optimize-away-classtags-and-array-…

737a649

…builders Optimize away ClassTags and ArrayBuilders

gzm0 merged commit 737a649 into scala-js:master May 20, 2015

sjrd deleted the optimize-away-classtags-and-array-builders branch May 20, 2015 20:15

sjrd mentioned this pull request Jul 15, 2016

Fix #2521: Update overrides of ClassTag and Manifest for 2.12. #2531

Merged

		@@ -0,0 +1,72 @@
		/* __ *\
		________ ___ / / ___ __ ____ Scala.js Test Suite

Optimize away ClassTags and ArrayBuilders #1669

Optimize away ClassTags and ArrayBuilders #1669

Conversation

sjrd commented May 16, 2015

sjrd commented May 16, 2015

scala-jenkins commented May 16, 2015

scala-jenkins commented May 16, 2015

scala-jenkins commented May 17, 2015

scala-jenkins commented May 17, 2015

sjrd commented May 17, 2015

scala-jenkins commented May 17, 2015

scala-jenkins commented May 17, 2015

scala-jenkins commented May 17, 2015

gzm0 commented May 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 commented May 17, 2015

gzm0 commented May 17, 2015

Choose a reason for hiding this comment

sjrd commented May 17, 2015

gzm0 commented May 17, 2015

sjrd commented May 17, 2015

gzm0 commented May 17, 2015

sjrd commented May 17, 2015

gzm0 commented May 17, 2015

gzm0 commented May 17, 2015

gzm0 commented May 17, 2015

sjrd commented May 17, 2015

gzm0 commented May 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 commented May 17, 2015

gzm0 commented May 17, 2015

sjrd commented May 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 commented May 18, 2015

sjrd commented May 18, 2015

scala-jenkins commented May 18, 2015

sjrd commented May 18, 2015

sjrd commented May 19, 2015

gzm0 commented May 20, 2015