Skip to content

Conversation

@lrytz
Copy link
Member

@lrytz lrytz commented Dec 16, 2014

See individual commit messages.

@scala-jenkins scala-jenkins added this to the 2.11.6 milestone Dec 16, 2014
@lrytz lrytz force-pushed the opt/classBTypeFromClassfile branch from 5f3e91d to e4416ad Compare December 16, 2014 19:46
@lrytz
Copy link
Member Author

lrytz commented Dec 16, 2014

-Ybackend:GenBCode build here (the jvm/unreachable failure is expected, see #4211). Review by @gkossakowski.

@gkossakowski
Copy link
Contributor

I'll be offline until January 7th. If a review is needed before that time, please reassign. Marry Christmas and Happy New Year! 🎄 🎁 🎆

@lrytz lrytz force-pushed the opt/classBTypeFromClassfile branch from 3bbf6f0 to e4416ad Compare January 9, 2015 22:07
@gkossakowski gkossakowski self-assigned this Jan 14, 2015
@gkossakowski
Copy link
Contributor

I've went through the whole code. Apart from the few nitpicks, it all looks good. When addressing the feedback please make sure to rebase. The PR doesn't merge cleanly anymore.

lrytz added 5 commits January 16, 2015 23:17
Each ClassBType is identified by its internalName, the fully qualified
JVM class name. Before this change, the name was stored in the `chrs`
array of the compiler name table (hash consed), with the idea to avoid
materializing the string.

However, we materialize the string anyway, because each ClassBType is
stored in the classBTypeFromInternalNameMap, indexed by the string.
If string equality turns out to be too slow we can use interning.

For the inliner, we read classes from bytecode and create ClassBTypes
for them. The names of these classes would not yet exist in the name
table, so the backend would need to be able to create new names. Using
Strings removes this dependency.
There's already the map classBTypeFromInternalNameMap in BTypes which
stores all ClassBTypes.
Introduces methods for textifying classes, methods, InsnLists and
individual AbstractInsnNodes.
This infrastructure is required for the inliner: when inlining code
from a classfile, the corresponding ClassBType is needed for various
things (eg access checks, InnerClass attribute).

The test creates two ClassBTypes for the same class: once using the
(unpickled) Symbol, once using the parsed ASM ClassNode, and verifies
that the two are the same.

There's a cleanup to the InnerClass attribute:

    object T { class Member; def foo = { class Local } }
    class T

For Java compatibility the InnerClass entry for Member says the class
is nested in T (not in the module class T$). We now make sure to add
that entry only to T, not to T$ (unless Member is actually referenced
in the classfile T$, in that case it will be added, as required).
@lrytz lrytz force-pushed the opt/classBTypeFromClassfile branch from e4416ad to 8c270de Compare January 16, 2015 22:18
  - Rename CodeRepository to ByteCodeRepository
  - Scaladoc on OptimizerReporting
  - Scaladoc on ByteCodeRepository
@lrytz lrytz force-pushed the opt/classBTypeFromClassfile branch from 8c270de to f3f9eb4 Compare January 16, 2015 22:21
@lrytz
Copy link
Member Author

lrytz commented Jan 16, 2015

Thanks for the review, @gkossakowski! Rebased on 2.11.x, reworded some commit messages, added one new commit that addresses the remaining feedback.

@gkossakowski
Copy link
Contributor

LGTM

Great work!

gkossakowski added a commit that referenced this pull request Jan 19, 2015
Construct ClassBTypes from parsed classfiles
@gkossakowski gkossakowski merged commit 783c5cc into scala:2.11.x Jan 19, 2015
@lrytz lrytz deleted the opt/classBTypeFromClassfile branch March 11, 2015 16:07
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.

4 participants