Slow memory leak in Dotty's parser

[Blaisorblade]

Splitting out of #1527 to give a clearer summary.

Dotty interns identifiers and never releases them, even if you drop the compiler instance, because the hashtable used for interning is stored in a static field. While this only leaks memory slowly, I'm not sure that's OK (though @odersky is not convinced this is a real problem). So if one wants to avoid this leak in Ensime/ScalaIDE, it seems one would have to load Dotty in a separate classloader.

I'm trying to anticipate whether this is indeed a problem, so pinging @fommil and @sschaef:

• Is that indeed a false problem for Ensime/ScalaIDE?
• If the problem is there, is loading Dotty in a separate classloader (and discarding it on different projects) acceptable for Ensime/ScalaIDE? Is that something they do today with Scalac?

[Blaisorblade]

For completeness, let me quote @odersky's response here too:

As long as you compile the same files, the nametable will not grow, since
everything is hashed. In my experience, even if you compile different
files, the name table grows very slowly. There are simply not so many
different names to deal with. So, it could become a problem at some point,
but right now I am not seeing it.

[fommil]

@Blaisorblade the reason I bring up https://github.com/fommil/class-monkey is because it is not possible to "discard" a classloader that uses scalac at present because of several locations where files are not closed correctly (fundamentally JarFile's finalizer is never called). Hopefully this is remedied in dotty. I have several workarounds for scalac, including an additional monkey patch in ensime itself https://github.com/ensime/ensime-server/blob/2.0/monkeys/src/main/scala-2.11/scala/reflect/io/ZipArchive.scala

[smarter]

@fommil Right now dotty relies on scala.reflect.io though we'd love to get rid of that dependency.

[Blaisorblade]

@fommil OK, I see that's relevant. Is that the only obstacle or is there more?

[fommil]

that's probably the biggest memory leak that I'm aware of with scalac. Another memory leak we've observed is when the PC just doesn't return and we have to start a new one, but the old one never dies (eating CPU and memory). Having the ability to really demand "look, here, just stop what you're doing old chap" would be fantastic.

[Blaisorblade]

Sounds like you'd like to "isolate" Scalac more. I feared it would be hard.* If we do that, there are fewer reasons to fix the slow leak discussed in this issue.

*EDIT: I mean: I feared that encapsulating the compiler from outside would be an annoyance for clients, in which case on Dotty would have to fix all memory leaks instead of relying on "compiler encapsulation". Relying on encapsulation would reduce non-functional requirements on Dotty (a bit à la Erlang).

[kiritsuku]

I would like to avoid leaking memory, even if it is only a little bit. It simply makes up-scaling of the compilation process more difficult. We already have problems with scalac because some resources are not correctly handled/freed. Especially code generation in our test suite is a pain because we have to ensure uniqueness of types/symbols manually. Resetting an existing scalac instance is simply not possible. Btw, we generate thousands of compilation units in tests, therefore even a small leak may have a larger impact.

Because of complexity overhead, we often don't reuse existing instances of scalac but recreate them. This adds some startup overhead though. However, if dotc is able to minimize the startup overhead, it can leak as much memory as it wants because then we can simply trash it whenever we want. But I'm not sure to which extend it is possible to minimize the startup overhead. The largest problem is that for large projects a lot of dependencies may need to be recompiled.

[Blaisorblade]

@sschaef Thanks—I don't claim to have a full overview of Dotty, but I'd guess this is better fixed earlier than later. But @odersky argued fixing this is not easy, so a few questions:

Btw, we generate thousands of compilation units in tests, therefore even a small leak may have a larger impact.

This leak is from interning names—Dotty stores forever (in an object field) all the unique names it ever saw, so reusing names won't cause leaks.

it can leak as much memory as it wants because then we can simply trash it whenever we want

That won't help with the leak under discussion, since the leaked memory is stored in an object field. That's why I talk about discarding classloaders—unloading a class is the only way to discard static members. (Storing this table in Context is also apparently a problem).

[retronym]

Discarding state by throwing away classloaders is problematic for performance, as you also throw away all the JITted code.

[retronym]

A pragmatic solution here would be to add a clear method to the name table. Obviously that would be thread-unsafe, and could only be used when no compilers are active.

@Blaisorblade What did you mean by:

Storing this table in Context is also apparently a problem

For large multi-project builds, one problem is that you end up with lots of duplication between the name tables of the compilers for each sub-project. Name table sharing would be worthwhile in those cases. This could be still be achieved without a global name table by passing in an existing name table when creating the root context for a new compiler.

[Blaisorblade]

A pragmatic solution here would be to add a clear method to the name table. Obviously that would be thread-unsafe, and could only be used when no compilers are active.

👍 💯
Important nitpick: "No active compiler" doesn't make it safe, but that'd be fixed by memory barriers to prevent reorderings by CPUs/caches/compilers (say, creating a happens-before edge via writes/reads to volatiles—clear would need to write to one, while creating a new compiler would have to read the same volatile). Is this worth mentioning or it is obvious here?

What did you mean by:

Storing this table in Context is also apparently a problem

I just referred (badly?) to Odersky's rebuttal of the proposal in the parent thread:
#1527 (comment)

Can't we just move the relevant hashtable to whatever the equivalent of
Global is?

No, names have conceptually larger scope than a single global or, in the
case of dotty, root context). I [tried] that early on in the dotty design and
it became a mess quickly.

[Blaisorblade]

A pragmatic solution here would be to add a clear method to the name table.

Forgot: Do we probably want a more generic API for clients, in case it later needs to clear other object fields?