8000 Improve performance of the backend by retronym · Pull Request #5820 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

Improve performance of the backend #5820

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 21, 2017
Merged

Improve performance of the backend #5820

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b3975a5
Optimize label defs finder in the backend
retronym Mar 15, 2017
7a6dc1a
Avoid excessive file stats during classfile writing
retronym Mar 15, 2017
455729e
Use AnyRefMap in labelReferences
retronym Mar 15, 2017
53dd4e4
Cache ClassSymbol.javaBinaryNameString
retronym Mar 13, 2017
1ae858c
Remove expensive assertion in the backend
retronym Mar 15, 2017
bebb188
Optimize method descriptor creation
retronym Mar 17, 2017
070ab67
Review feedback: resurrect assertion, use LabelDefFinder.apply
retronym Apr 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Optimize method descriptor creation 
Thread a single StringBuilder through the component's stringification
methods, rather than using string concat (ie, separates StringBuilders)
at each level.
  • Loading branch information
@retronym
retronym committed Apr 3, 2017
commit bebb1886de7841f99e101e924f51b605735401e1
36 changes: 23 additions & 13 deletions src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Copy link
Contributor
@mkeskells mkeskells Apr 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 about this coordination

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lrytz split out several PRs for this #5831 #5834 #5833 #5832

haven't test them or got he performance figures - will run overnight and post up if successful

val builder = new java.lang.StringBuilder(64)
buildString(builder)
builder.toString
}

final def buildString(builder: java.lang.StringBuilder): Unit = this match {
case UNIT => builder.append('V')
case BOOL => builder.append('Z')
case CHAR => builder.append('C')
case BYTE => builder.append('B')
case SHORT => builder.append('S')
case INT => builder.append('I')
case FLOAT => builder.append('F')
case LONG => builder.append('J')
case DOUBLE => builder.append('D')
case ClassBType(internalName) => builder.append('L').append(internalName).append(';')
case ArrayBType(component) => builder.append('['); component.buildString(builder)
case MethodBType(args, res) =>
builder.append('(')
args.foreach(_.buildString(builder))
builder.append(')')
res.buildString(builder)
}

/**
Expand Down
0