Skip to content

better way to look for method bodies to inline, helps with SI-4767 #849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

magarciaEPFL
Copy link
Contributor

This pull request changes a core aspect of the inliner, thus please be sure to re-check.

At least one aspect definitely needs improvement: the definition of isJDKClass(). Ideas are specially welcome about that.

As well as comments about any material effect on performance ( @gkossakowski comes to mind ).

review by @VladUreche
review by @dragos

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/452/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/452/

@magarciaEPFL
Copy link
Contributor Author

Let's see if the above was an intermittent failure.
PLS REBUILD ALL

@scala-jenkins
Copy link

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/454/

@scala-jenkins
Copy link

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/454/

@@ -160,6 +160,10 @@ abstract class Inliners extends SubComponent {
def hasInline(sym: Symbol) = sym hasAnnotation ScalaInlineClass
def hasNoInline(sym: Symbol) = sym hasAnnotation ScalaNoInlineClass

def isJDKClass(csym: Symbol) = csym.isJavaDefined && csym.fullName.startsWith("java") // TODO this needs improvement
Copy link
Contributor

Choose a reason for hiding this comment

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

What property exactly is it trying to identify? Here's an implementation which should under normal conditions identify anything which originates in a jar under the running vm. How exactly this will interact with symlinks etc. I don't know.

class JdkClassIdentifier extends (String => Boolean) {
  private val javaHome         = sys.props("java.home")
  private val loader           = new URLClassLoader(Array[URL]())
  // Since I'm not sure of the intent behind the code I'm
  // writing, I don't know whether pre-filtering based on
  // package names is desirable.
  private val knownJdkPackages = "java javax com.sun sun" split ' ' map (_ + ".") toVector

  def apply[T](implicit ctag: reflect.ClassTag[T]): Boolean = apply(ctag.runtimeClass.getName)
  def apply(name: String): Boolean = {
    knownJdkPackages.exists(name startsWith _) || {
      val path = name.replace('.', '/') + ".class"
      loader getResource path match {
        case null => false
        case url  => 
          // At this point it looks something like
          //    jar:file:/Library/Java/JavaVirtualMachines/1.7.0.jdk/Contents/Home/jre/lib/rt.jar!/java/lang/String.class
          // getPath drops "jar:" so we remove "file:" - presumably if there is no jar
          // involved, getPath will drop "file:" instead.
          url.getPath stripPrefix "file:" startsWith javaHome
      }
    }
  }
}

@magarciaEPFL
Copy link
Contributor Author

Thanks for the discussion, it will help me in fixing the issues uncovered by the build failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants