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

Optimizer leaves references to classes that have been eliminated by inlining #6546

Closed
scabug opened this Issue Oct 19, 2012 · 11 comments

Comments

Projects
None yet
2 participants
@scabug
Copy link

scabug commented Oct 19, 2012

I am running into a problem where the optimizer is not completely removing references to inner classes it has decided it doesn't need to output. This is a serious problem in JavaEE 6 environments that rely heavily on class detection. I don't know of any workaround if you want to use Scala + JavaEE 6 except to turn off inlining, which is a pretty severe performance penalty in some cases.

The problem is all due to the inliner. All the little inner classes that get generated to represent anonymous functions, inner objects, etc. are smartly subject to removal if they can be inlined. I like this feature, as it does a good job speeding up startup and reducing the PermGen cost of Scala.

However, the problem is that the inliner does not remove these elided classes from the inner-classes declaration that is part of the class file format for outer classes. This inner-classes declaration is a key part of how class scanners look for all annotated classes, which is a major part of JavaEE 6 containers. The container ends up complaining that some of your declared classes are not present because there's no physical .class file. In JBoss, one such error prevents your entire module from deploying as it fails verification.

Even though these inner classes are never required by any actual method bytecode, the compiler is wrong to declare that they exist. I've also seen it cause warnings in other tools, though this is the worst symptom I've seen. I'm hoping that we can get for this as part of 2.10 if not sooner. (I haven't looked to see if this issue is still present in the 2.10 line. Last time I tried using the 2.10 milestones it didn't work for me.)

Besides the poor Eclipse performance, the implications of no optimization is one of the issues that is raising concerns about further adoption of Scala. :-)

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 19, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6546?orig=1
Reporter: Sarah Gerweck (gerweck)
Affected Versions: 2.9.2, 2.10.0-RC1

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 19, 2012

@paulp said:
It's probably too late for 2.10, but I agree it's bad news. It shouldn't be hard to fix, for 2.10.1 if not 2.10.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 24, 2012

@magarciaEPFL said (edited on Oct 24, 2012 8:10:16 PM UTC):
Diagnosis: In GenASM.run() those closure classes that aren't "live" anymore are removed from the to-be-emitted list:

      if (settings.Xdce.value)
        for ((sym, cls) <- icodes.classes if inliner.isClosureClass(sym) && !deadCode.liveClosures(sym)) {
          log(s"Optimizer eliminated ${sym.fullNameString}")
          icodes.classes -= sym
        }

However in JBuilder.addInnerClasses() that information is overlooked:

      // add inner classes which might not have been referenced yet
      exitingErasure {
        for (sym <- List(csym, csym.linkedClassOfClass); m <- sym.info.decls.map(innerClassSymbolFor) if m.isClass)
          innerClassBuffer += m
      }

Candidate solution, below.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 24, 2012

@magarciaEPFL said:
Let's keep track of elided closures classes in a mutable set of symbols :

      if (settings.Xdce.value)
        for ((sym, cls) <- icodes.classes if inliner.isClosureClass(sym) && !deadCode.liveClosures(sym)) {
          log(s"Optimizer eliminated ${sym.fullNameString}")
          elidedClosureClasses += sym  // <---------- part 1 of 2
          icodes.classes -= sym
        }

With that,

      // add inner classes which might not have been referenced yet
      exitingErasure {
        for (sym <- List(csym, csym.linkedClassOfClass);
             m <- sym.info.decls.map(innerClassSymbolFor)
             if m.isClass && !elidedClosureClasses(sym))   // <---------- part 2 of 2
          innerClassBuffer += m
      }
@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 26, 2012

Sarah Gerweck (gerweck) said:
Since it looks like this is a pretty straightforward patch, I'd like to make one more plea to that it be fixed for 2.10. At least for me, this issue has some pretty significant consequences. I'm sure you guys have process to deal with and I completely understand if you can't do it, but I figured I'd ask. :-)

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 28, 2012

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 29, 2012

@magarciaEPFL said:
Pull requrest closed by @gkossakowski so the fix won't show up in 2.10.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 14, 2012

Sarah Gerweck (gerweck) said:
Thanks for trying anyway. :-)

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Jan 9, 2013

Sarah Gerweck (gerweck) said:
Any chance this will be part of 2.10.1? Is it a question of just needing someone to figure out the solution? This continues to be a really painful issue for me, as it means we cannot use optimization.

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Oct 31, 2013

@scabug

This comment has been minimized.

Copy link
Author

scabug commented Nov 5, 2013

@paulp said:
I submitted it for 2.10.4 as scala/scala#3097 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment