Improve performance of the backend #5820
Conversation
- Record the entry for the RHS of the DefDef is a dedicated
field to avoid immediately looking it up in a hash map after
traversal
- Use an AnyRefMap to avoid BoxesRuntime hashCode/equals
- Use getOrElse rather than withDefaultValue to profit from a fast
path in AnyRefMap.
The existing implementation pessimistically checks that all parent directories of the about-to-be-written class file are indeed directories. This commit bypasses this logic for the common case of writing to a regular directory on disk, and optimistically assumes that the parent directory exists. If an exception is thrown during writing, it attempts to create the parent directory. This still avoids a compiler crash if a parent directory is actually a file, which is tested by the existing test, `run/t5717.scala`.
This is a hot method in the backend, and we save some cycles by avoiding BoxesRuntime.
The backend uses this string as a key to the map of BTypes, and as such needs to call `javaBinaryNameString` for each method or field reference in the code. Even though we have previously optimized the creation of this string by bypassing the Name abstraction and by correctly sizing string builders, we can still speed things up by caching the resulting String on its ClassSymbol.
These assertions don't seem to pay their way anymore, the surrounding implementation of the backend has matured, and they involve collections operations that are too costly to be called in such a hot path in the backend.
Thread a single StringBuilder through the component's stringification methods, rather than using string concat (ie, separates StringBuilders) at each level.
| @@ -461,10 +461,10 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { | |||
| locals.reset(isStaticMethod = methSymbol.isStaticMember) | |||
| jumpDest = immutable.Map.empty[ /* LabelDef */ Symbol, asm.Label ] | |||
| // populate labelDefsAtOrUnder | |||
| val ldf = new LabelDefsFinder | |||
| val ldf = new LabelDefsFinder(dd.rhs) | |||
| ldf.traverse(dd.rhs) | |||
| try { | ||
| Files.write(outfile.file.toPath, jclassBytes) | ||
| } catch { | ||
| case _: java.nio.file.NoSuchFileException => |
There was a problem hiding this comment.
would it be too slow to check java.nio.file.Files.exists(p.getParent)?
There was a problem hiding this comment.
I found some small but measurable improvement from this approach, as there are typically many classfiles per directory, which makes the optimism pays off. I could split this into two commits so that we could reproduce that measurement.
There was a problem hiding this comment.
I think that there is a better solution in #5815, that will only check a given directory once, and will not call createDirectories unless it the first time that the directory is reached
There was a problem hiding this comment.
@lrytz each stat call is expensive in windows particually
There was a problem hiding this comment.
I like the approach of caching the results of file stats, but opted not to do that in this PR because it takes a bit of care to get the lifecycle of that cache sorted out (we need to start fresh on a new compiler run, but the current design of ByteCodeWriters didn't have a hook for that part of the lifecycle.)
There was a problem hiding this comment.
@retronym it is hooked in this my PR as the OutputDirectories are created new for each run
There was a problem hiding this comment.
I have split out an alternative as #5834
will run performance test overnight and post up is successful
| @@ -108,8 +108,6 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes { | |||
|
|
|||
| assert(classSym != NoSymbol, "Cannot create ClassBType from NoSymbol") | |||
| assert(classSym.isClass, s"Cannot create ClassBType from non-class symbol $classSym") | |||
| assertClassNotArrayNotPrimitive(classSym) | |||
| assert(!primitiveTypeToBType.contains(classSym) || isCompilingPrimitive, s"Cannot create ClassBType for primitive class symbol $classSym") | |||
There was a problem hiding this comment.
I'm a bit sad to see those go.. They were helpful for finding bugs, and they are also useful documentation. Can we put them under a flag or elide them? Or even leave them as comments.
There was a problem hiding this comment.
I'll resurrect under -Xdev (which is also enabled by -Ydebug)
|
|
/rebuild |
| case ClassBType(internalName) => "L" + internalName + ";" | ||
| case ArrayBType(component) => "[" + component | ||
| case MethodBType(args, res) => "(" + args.mkString + ")" + res | ||
| final override def toString: String = { |
There was a problem hiding this comment.
this is non optimal still
There is a better version I think in https://github.com/scala/scala/pull/5815/files#diff-61481becb11478d78c0b577ecad981aaR320
that version caches the result and shares the (java) StringBuilder when building a compound type, and avoid builder generation for primative types
There was a problem hiding this comment.
@mkeskells it would be good to extract small, self-contained pieces of #5815 that can be submitted as individual PRs. This will make reviewing much easier and also avoid duplicate work.
There was a problem hiding this comment.
Caching all the intermediate strings is an interesting idea. Usually I'd be a bit cautious about that for a recursive structure, as it requires exponential space wrt the depth of the tree. BTypes, however, only have "deep" trees for higher dimensional arrays, so this might not be a practical concern here.
There was a problem hiding this comment.
@retronym should be easy to bake off as we have the profiler and this is a measurable part of the backend CPU, and we have stable allocation counters
Not sure if the allocation counters are in the version that you have
|
I'm looking into options for that. Will get back to you on that thread.
…On Wed, Apr 5, 2017 at 13:15 mkeskells ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
<#5820 (comment)>:
> @@ -298,19 +298,29 @@ abstract class BTypes {
* referring to BTypes.
*/
sealed trait BType {
- final override def toString: String = this match {
- case UNIT => "V"
- case BOOL => "Z"
- case CHAR => "C"
- case BYTE => "B"
- case SHORT => "S"
- case INT => "I"
- case FLOAT => "F"
- case LONG => "J"
- case DOUBLE => "D"
- case ClassBType(internalName) => "L" + internalName + ";"
- case ArrayBType(component) => "[" + component
- case MethodBType(args, res) => "(" + args.mkString + ")" + res
+ final override def toString: String = {
@lrytz <https://github.com/lrytz> I will be working on that but I also
have a day job. I was trying to get more tests to pass
I have seperately opened a discussion with @adriaanm
<https://github.com/adriaanm> about this coordination
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5820 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFjy2iumuL9J3gn0mYxjtFxueRF9Uz0ks5rs_ZIgaJpZM4MxHR->
.
|
There was a problem hiding this comment.
This LGTM, but it overlaps with @mkeskells' #5831 (BType descriptor generation) and #5834 (Classfile writing IO). These PRs are more comprehensive than the corresponding changes here. We can either remove the two commits here or rebase the other PRs.
|
My impression is that both of those PRs are worth pursuing, but that the initial versions herein will be a good starting point (simpler code changes / deliver much of the performance improvement), so it would make sense to merge this and rebase the other changes. That way, if we have to revert the more involved changes due to a regression or undesirable performance consequence, we'd be back to square 2, instead of square 1. |
|
Sounds good to me. |
In aggregate, I measure a 3.5% improvement from this PR.