8000 optimise Btypes descriptor generation by mkeskells · Pull Request #5831 · scala/scala · GitHub
[go: up one dir, main page]
Skip to content

optimise Btypes descriptor generation #5831

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 yo 8000 ur account

Closed
wants to merge 2 commits into from
Closed

optimise Btypes descriptor generation #5831

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f57531
cache Btypes descriptor
mkeskells Apr 5, 2017
14132b4
cache toASMType
mkeskells Apr 5, 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
161 changes: 106 additions & 55 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 @@ -305,39 +305,23 @@ abstract class BTypes {
* referring to BTypes.
*/
sealed trait BType {
final override def toString: String = {
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)
}
final override def toString: String = descriptor

/**
* @return The Java descriptor of this type. Examples:
* - int: I
* - java.lang.String: Ljava/lang/String;
* - int[]: [I
* - Object m(String s, double d): (Ljava/lang/String;D)Ljava/lang/Object;
*/
final def descriptor = toString
* @return The Java descriptor of this type. Examples:
* - int: I
* - java.lang.String: Ljava/lang/String;
* - int[]: [I
* - Object m(String s, double d): (Ljava/lang/String;D)Ljava/lang/Object;
*/
def descriptor : String

/**
* generally used by descriptor to help build the descriptor. Allows several descriptors to be build from the same stringbuilder
* @param sb
*/
private[BTypes] def buildDescriptor(sb: java.lang.StringBuilder): Unit
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to use a pattern match here instead of virtual dispatch. With an overload override def descriptor(sb: java.lang.StringBuilder): String in Class/Array/MethodBType this should be possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you are getting at
As I see it you either use a pattern match or virtual dispatch
Getting run of the 'this match { case ...} ' pattern in the code aids encapsulation, safety and performance IMHO

Why an overload? They are behaviourly different?

I am turn as to whether the buildDescriptor should return the descriptor or generate i as a side effect. Either way there are 2 effects, and I could not come up with a a better name before my attention moved

Copy link
Member

Choose a reason for hiding this comment

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

I think we generally prefer matching on this for sealed data types like BType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lrytz I think that it is a very bad idea. I did see large amounts or this elsewhere, and it is on my todo list. Is this personal preference or lightbend policy?

My view -

Given that you dont have warnings as fatal, it is fragile

From a perfomance perspective I would expect it to be non-optimal. Has this been benchmarked? I know that @retronym is a dab hand with JMH :-)
I would suggest that we need some figures to compare and determine what the effective costs and benefits are

Virtual dispatch, particularly from abstract classes rather than traits is very fast. Pattern matching is a sequential operation, slowed with unapply calls. Inner classes (e.g., from the global cake) make this even more onerous

I agree that is some cases it may be appropriate - code share, cold paths, but these paths are hot

Copy link
Member
@retronym retronym Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Pattern matching is a sequential operation, slowed with unapply calls

In general, your point is valid, and in some cases the performance does make a difference (e.g. in mapOver #5906 ), but it is important to note that most pattern matching is on case class constructors or plain old instanceOfs and doesn't involve unapply calls. So we should balance readability (which is sometimes enhanced with a pattern match) vs performance by confirming that there is an improvement in benchmarks from a refactoring.


/**
* @return 0 for void, 2 for long and double, 1 otherwise
Expand Down Expand Up @@ -494,28 +478,20 @@ abstract class BTypes {
* - for an OBJECT type, the 'L' and ';' are not part of the range of the created Type
* - for an ARRAY type, the full descriptor is part of the range
*/
def toASMType: asm.Type = this match {
case UNIT => asm.Type.VOID_TYPE
case BOOL => asm.Type.BOOLEAN_TYPE
case CHAR => asm.Type.CHAR_TYPE
case BYTE => asm.Type.BYTE_TYPE
case SHORT => asm.Type.SHORT_TYPE
case INT => asm.Type.INT_TYPE
case FLOAT => asm.Type.FLOAT_TYPE
case LONG => asm.Type.LONG_TYPE
case DOUBLE => asm.Type.DOUBLE_TYPE
case ClassBType(internalName) => asm.Type.getObjectType(internalName) // see (*) above
case a: ArrayBType => asm.Type.getObjectType(a.descriptor)
case m: MethodBType => asm.Type.getMethodType(m.descriptor)
}
def toASMType: asm.Type

def asRefBType : RefBType = this.asInstanceOf[RefBType]
def asArrayBType : ArrayBType = this.asInstanceOf[ArrayBType]
def asClassBType : ClassBType = this.asInstanceOf[ClassBType]
def asPrimitiveBType : PrimitiveBType = this.asInstanceOf[PrimitiveBType]
}

sealed trait PrimitiveBType extends BType {
sealed abstract class PrimitiveBType(
override val descriptor:String,
override val toASMType: asm.Type) extends BType {
private[BTypes] override def buildDescriptor(sb: java.lang.StringBuilder): Unit = {
sb.append(descriptor)
}

/**
* The upper bound of two primitive types. The `other` type has to be either a primitive
Expand Down Expand Up @@ -580,15 +556,15 @@ abstract class BTypes {
}
}

case object UNIT extends PrimitiveBType
case object BOOL extends PrimitiveBType
case object CHAR extends PrimitiveBType
case object BYTE extends PrimitiveBType
case object SHORT extends PrimitiveBType
case object INT extends PrimitiveBType
case object FLOAT extends PrimitiveBType
case object LONG extends PrimitiveBType
case object DOUBLE extends PrimitiveBType
case object UNIT extends PrimitiveBType("V",asm.Type.VOID_TYPE)
case object BOOL extends PrimitiveBType("Z",asm.Type.BOOLEAN_TYPE)
case object CHAR extends PrimitiveBType("C",asm.Type.CHAR_TYPE)
case object BYTE extends PrimitiveBType("B",asm.Type.BYTE_TYPE)
case object SHORT extends PrimitiveBType("S",asm.Type.SHORT_TYPE)
case object INT extends PrimitiveBType("I",asm.Type.INT_TYPE)
case object FLOAT extends PrimitiveBType("F",asm.Type.FLOAT_TYPE)
case object LONG extends PrimitiveBType("J",asm.Type.LONG_TYPE)
case object DOUBLE extends PrimitiveBType("D",asm.Type.DOUBLE_TYPE)

sealed trait RefBType extends BType {
/**
Expand Down Expand Up @@ -865,6 +841,34 @@ abstract class BTypes {
* infos using `get`, but it reports inliner warnings for missing infos that prevent inlining.
*/
final case class ClassBType(internalName: InternalName)(cache: mutable.Map[InternalName, ClassBType]) extends RefBType {

override def descriptor: String = {
if (descriptorCache eq null) {
val sb = new java.lang.StringBuilder(internalName.length + 2)
buildDescriptor(sb)
Copy link
Member

Choose a reason for hiding this comment

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

there will probably be concurrent accesses to this method once we parallelize the backend, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it would be concurrent, but I believe that it is threadsafe

I don't believe that the cost of lock is worth the potential gain in lost CPU

the answer is the same (==) so 2 threads race one will win. Small chance that we will have 2 thread with identical strings, but this is not harmful

There is no chance of a incomplete string being visible to another thread as String constructor has a memory fence

}
descriptorCache
}
private var descriptorCache : String = _
private[BTypes] override def buildDescriptor(sb: java.lang.StringBuilder): Unit = if (descriptorCache eq null) {
val start = sb.length()
sb.append('L')
sb.append(internalName)
sb.append(';')

descriptorCache = sb.substring(start)
} else sb.append(descriptorCache)


/**
* The asm.Type corresponding to this BType.
*
* Note about asm.Type.getObjectType (*): For class types, the method expects the internal
* name, i.e. without the surrounding 'L' and ';'.
*/
override lazy val toASMType = asm.Type.getObjectType(internalName)


/**
* Write-once variable allows initializing a cyclic graph of infos. This is required for
* nested classes. Example: for the definition `class A { class B }` we have
Expand Down Expand Up @@ -1160,6 +1164,31 @@ abstract class BTypes {
final case class InnerClassEntry(name: String, outerName: String, innerName: String, flags: Int)

final case class ArrayBType(componentType: BType) extends RefBType {
override def descriptor: String = {
if (descriptorCache eq null) {
val sb = new java.lang.StringBuilder(256)
buildDescriptor(sb)
}
descriptorCache
}
private var descriptorCache : String = _
private[BTypes] override def buildDescriptor(sb: java.lang.StringBuilder): Unit = if (descriptorCache eq null) {
val start = sb.length()
sb.append('[')
componentType.buildDescriptor(sb)

descriptorCache = sb.substring(start)
} else sb.append(descriptorCache)


/**
* For array types on the other hand, the method expects a full descriptor,
* for example "[Ljava/lang/String;".
*
*/
override lazy val toASMType = asm.Type.getObjectType(descriptor)


def dimension: Int = componentType match {
case a: ArrayBType => 1 + a.dimension
case _ => 1
Expand All @@ -1171,7 +1200,29 @@ abstract class BTypes {
}
}

final case class MethodBType(argumentTypes: List[BType], returnType: BType) extends BType
final case class MethodBType(argumentTypes: List[BType], returnType: BType) extends BType {
override def descriptor: String = {
if (descriptorCache eq null) {
val sb = new java.lang.StringBuilder(256)
buildDescriptor(sb)
}
descriptorCache
}

private var descriptorCache: String = _

private[BTypes] override def buildDescriptor(sb: java.lang.StringBuilder): Unit = if (descriptorCache eq null) {
val start = sb.length()
sb.append('(')
argumentTypes foreach (_.buildDescriptor(sb))
sb.append(')')
returnType.buildDescriptor(sb)
descriptorCache = sb.substring(start)

} else sb.append(descriptorCache)

override lazy val toASMType = asm.Type.getMethodType(descriptor)
}

/* Some definitions that are required for the implementation of BTypes. They are abstract because
* initializing them requires information from types / symbols, which is not accessible here in
Expand Down
0