Skip to content
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

Review/8168 #51

Closed
wants to merge 8 commits into from
Closed

Review/8168 #51

wants to merge 8 commits into from

Conversation

retronym
Copy link
Owner

No description provided.

retronym and others added 8 commits June 19, 2019 18:21
------------------------------------------------------------------------

Reuse the buffer for classfile reading

Classfile parsing does re-enter when we're reading package objects
or classfiles for things like `scala/native.class`.

But for the most part the prior refactorings mean that we typically
only parse a single classfile at a time, and as such we can profit
from a one-element cache for the buffer to read this into.

(cherry picked from commit ed8d95e)

------------------------------------------------------------------------

Eagerly read from the constant pool as a basis for lazy types java class/method

I've used lazy types for field/method/class infos, which is analagous to what we
do in `Unpickler` for scala originated types.  We read all data needed by the
inner class table and the type completers from the pool eagerly, but still be
lazy about interning strings to Names and completion of the field/method
types themselves.

This fixes some long standing spurious cyclic errors:

Manually tested with:

```
$ scalac -cp $(coursier fetch -q -p com.datastax.cassandra:dse-driver:1.0.0) test.scala
test.scala:2: error: illegal cyclic reference involving class Cluster
        new com.datastax.driver.dse.DseCluster.Builder()
                                               ^
one error found

$ /code/scala/build/quick/bin/scalac -cp $(coursier fetch -q -p com.datastax.cassandra:dse-driver:1.0.0) test.scala

$ cat test.scala
class Test {
        new com.datastax.driver.dse.DseCluster.Builder()
}
```

------------------------------------------------------------------------

Avoid using Names for fully qualified class names

There is no good reason for these dotted names to be
Names and stick around in the name table. Let's use
short lived strings instead.

Reduces the name table by 5% in terms of entries and 10%
in terms of characters when compiling src/scalap/**/*.scala

(cherry picked from commit ae18049)

------------------------------------------------------------------------

Avoid Names for descriptors, generic sigs, and string constants

We can just keep these are short-lived Strings, rather than interning
them into the Name table for the entire lifetime of Global.

(cherry picked from commit 688bf0f)

------------------------------------------------------------------------

Invalidate symbols for artifact classfiles, refactor classfile parser

No longer run the classfile parser on Scala generated classfiles that
don't have a Scala signature (module classes, inner classes, etc).

Various cleanups in the classfile parser, minimize the work performed
on Scala classfiles. Before, the attributes section was parsed twice:
once to find the ScalaSig attribute, the second time to find the
ScalaSignature in the RuntimeVisibleAnnotations. Now everything happens
in the first iteration.

Also fixes a bug in the backend: classes ending in `$` did not get a
ScalaSignature by mistake. They were filtered out by the name-based
test that is supposed to identify module classes.

(cherry picked from commit 3aea776)

------------------------------------------------------------------------

Remove unnecessary abstraction

Added in ced7214, no longer needed since ICodeReader is gone.

(cherry picked from commit e216e0e)
remove unused local and optimise locals uasge
add some more tests
add some more benchmarks
add test assertions on allocations.
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.

2 participants