Skip to content

Commit

Permalink
scrooge-generator - Optimize Scrooge-generated Scala classes from thrift
Browse files Browse the repository at this point in the history 
Problem

Scala classes generated by Scrooge in Scala take a long time to compile and
generate inefficient bytecode which can hurt likelihood of JVM inlining methods.

Solution

Remove 'match' which does not translate into a tableswitch in favor of if-else,
remove most uses of implicit functions, shadow variables to generate smaller
bytecode, eliminate some redundant methods, move common functionality around
throwing exceptions into an internal ApplicationExceptions class.

Result

Compile times for Scrooge-generated Scala times are improved, and smaller
bytecode is generated for key methods.

JIRA Issues: CSL-8870

Differential Revision: https://phabricator.twitter.biz/D454297
  • Loading branch information
@michaelbraun
michaelbraun authored and jenkins committed Apr 21, 2020
1 parent b72a08c commit 2189d28
Show file tree
Hide file tree
Showing 36 changed files with 2,500 additions and 2,828 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ Unreleased
* scrooge-generator: Respect the proper order of separators in function declarations.
``PHAB_ID=D467476``

* scrooge-generator: Optimized generated Scala code for compile time and smaller bytecode.
Companion objects for thrift enum traits are no longer case objects. ``PHAB_ID=D454297``

20.4.0
------

Expand Down
42 changes: 42 additions & 0 deletions scrooge-core/src/main/scala/com/twitter/scrooge/internal/ApplicationExceptions.scala
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package com.twitter.scrooge.internal

import com.twitter.scrooge.ThriftStruct
import org.apache.thrift.TApplicationException
import org.apache.thrift.protocol.TProtocolException

/**
* Internal helpers for Scrooge classes - should not be called by user code
*/
object ApplicationExceptions {

/**
* Given a service name, return a TApplicationException of type MISSING_RESULT
* where the message includes the service name
* @param serviceName
* @return
*/
def missingResult(serviceName: String): TApplicationException = {
new TApplicationException(
TApplicationException.MISSING_RESULT,
serviceName + " failed: unknown result"
)
}

/**
* Given a message and the expected and actual type (byte representation), throws a
* TProtocolException - will call String.format on the message with the ttypeToString of the
* expected and actual type bytes as the two args.
* @param message
* @param expectedType
* @param actualType
*/
@throws[TProtocolException]
def throwWrongFieldTypeException(message: String, expectedType: Byte, actualType: Byte): Unit = {
throw new TProtocolException(
String.format(
message,
ThriftStruct.ttypeToString(expectedType),
ThriftStruct.ttypeToString(actualType)))
}

}
151 changes: 66 additions & 85 deletions ...s/gold_file_output_scala/com/twitter/scrooge/test/gold/thriftscala/AnotherException.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ object AnotherException extends ValidatingThriftStructCodec3[AnotherException] w
val NoPassthroughFields: immutable$Map[Short, TFieldBlob] = immutable$Map.empty[Short, TFieldBlob]
val Struct: TStruct = new TStruct("AnotherException")
val ErrorCodeField: TField = new TField("errorCode", TType.I32, 1)
val ErrorCodeFieldManifest: Manifest[Int] = implicitly[Manifest[Int]]
val ErrorCodeFieldManifest: Manifest[Int] = manifest[Int]

/**
* Field information in declaration order.
Expand All @@ -49,26 +49,25 @@ object AnotherException extends ValidatingThriftStructCodec3[AnotherException] w
)
)

lazy val structAnnotations: immutable$Map[String, String] =

val structAnnotations: immutable$Map[String, String] =
immutable$Map.empty[String, String]

private val fieldTypes: IndexedSeq[ClassTag[_]] = IndexedSeq(
private val fieldTypes: IndexedSeq[ClassTag[_]] = IndexedSeq[ClassTag[_]](
classTag[Int].asInstanceOf[ClassTag[_]]
)

private[this] val structFields: Seq[ThriftStructField[AnotherException]] = {
Seq(
new ThriftStructField[AnotherException](
ErrorCodeField,
_root_.scala.Some(ErrorCodeFieldManifest),
classOf[AnotherException]) {
def getValue[R](struct: AnotherException): R = struct.errorCode.asInstanceOf[R]
}
)
}
private[this] val structFields: Seq[ThriftStructField[AnotherException]] = Seq[ThriftStructField[AnotherException]](
new ThriftStructField[AnotherException](
ErrorCodeField,
_root_.scala.Some(ErrorCodeFieldManifest),
classOf[AnotherException]) {
def getValue[R](struct: AnotherException): R = struct.errorCode.asInstanceOf[R]
}
)

override lazy val metaData: ThriftStructMetaData[AnotherException] =
new ThriftStructMetaData(this, structFields, fieldInfos, Seq(), structAnnotations)
new ThriftStructMetaData(this, structFields, fieldInfos, Nil, structAnnotations)

/**
* Checks that all required fields are non-null.
Expand All @@ -89,11 +88,7 @@ object AnotherException extends ValidatingThriftStructCodec3[AnotherException] w

def withoutPassthroughFields(original: AnotherException): AnotherException =
new AnotherException(
errorCode =
{
val field = original.errorCode
field
}
errorCode = original.errorCode
)

def newBuilder(): StructBuilder[AnotherException] = new AnotherExceptionStructBuilder(_root_.scala.None, fieldTypes)
Expand All @@ -109,38 +104,36 @@ object AnotherException extends ValidatingThriftStructCodec3[AnotherException] w
var _done = false

_iprot.readStructBegin()
while (!_done) {
do {
val _field = _iprot.readFieldBegin()
if (_field.`type` == TType.STOP) {
val _fieldType = _field.`type`
if (_fieldType == TType.STOP) {
_done = true
} else {
_field.id match {
case 1 =>
_field.`type` match {
case TType.I32 =>
errorCode = readErrorCodeValue(_iprot)
case _actualType =>
val _expectedType = TType.I32
throw new TProtocolException(
"Received wrong type for field 'errorCode' (expected=%s, actual=%s).".format(
ttypeToString(_expectedType),
ttypeToString(_actualType)
)
)
if (_fieldType == TType.I32) {
errorCode = readErrorCodeValue(_iprot)
} else {
_root_.com.twitter.scrooge.internal.ApplicationExceptions.throwWrongFieldTypeException(
"Received wrong type for field 'errorCode' (expected=%s, actual=%s).",
TType.I32,
_fieldType
)
}
case _ =>
if (_passthroughFields == null)
if (_passthroughFields eq null)
_passthroughFields = immutable$Map.newBuilder[Short, TFieldBlob]
_passthroughFields += (_field.id -> TFieldBlob.read(_field, _iprot))
_passthroughFields += _root_.scala.Tuple2(_field.id, TFieldBlob.read(_field, _iprot))
}
_iprot.readFieldEnd()
}
}
} while (!_done)
_iprot.readStructEnd()

new AnotherException(
errorCode,
if (_passthroughFields == null)
if (_passthroughFields eq null)
NoPassthroughFields
else
_passthroughFields.result()
Expand Down Expand Up @@ -216,37 +209,33 @@ class AnotherException(
* is known and not optional and set to None, then the field is serialized and returned.
*/
def getFieldBlob(_fieldId: Short): _root_.scala.Option[TFieldBlob] = {
lazy val _buff = new TMemoryBuffer(32)
lazy val _oprot = new TCompactProtocol(_buff)
_passthroughFields.get(_fieldId) match {
case blob: _root_.scala.Some[TFieldBlob] => blob
case _root_.scala.None => {
val _fieldOpt: _root_.scala.Option[TField] =
_fieldId match {
case 1 =>
if (true) {
writeErrorCodeValue(errorCode, _oprot)
_root_.scala.Some(AnotherException.ErrorCodeField)
} else {
_root_.scala.None
}
case _ => _root_.scala.None
}
_fieldOpt match {
case _root_.scala.Some(_field) =>
_root_.scala.Some(TFieldBlob(_field, Buf.ByteArray.Owned(_buff.getArray())))
case _root_.scala.None =>
_root_.scala.None
}
val passedthroughValue = _passthroughFields.get(_fieldId)
if (passedthroughValue.isDefined) {
passedthroughValue
} else {
val _buff = new TMemoryBuffer(32)
val _oprot = new TCompactProtocol(_buff)

val _fieldOpt: _root_.scala.Option[TField] = _fieldId match {
case 1 =>
writeErrorCodeValue(errorCode, _oprot)
_root_.scala.Some(AnotherException.ErrorCodeField)
case _ => _root_.scala.None
}
if (_fieldOpt.isDefined) {
_root_.scala.Some(TFieldBlob(_fieldOpt.get, Buf.ByteArray.Owned(_buff.getArray)))
} else {
_root_.scala.None
}
}
}


/**
* Collects TCompactProtocol-encoded field values according to `getFieldBlob` into a map.
*/
def getFieldBlobs(ids: TraversableOnce[Short]): immutable$Map[Short, TFieldBlob] =
(ids flatMap { id => getFieldBlob(id) map { id -> _ } }).toMap
(ids.flatMap { id => getFieldBlob(id).map { fieldBlob => (id, fieldBlob) } }).toMap

/**
* Sets a field using a TCompactProtocol-encoded binary blob. If the field is a known
Expand All @@ -260,7 +249,7 @@ class AnotherException(
_blob.id match {
case 1 =>
errorCode = readErrorCodeValue(_blob.read)
case _ => _passthroughFields += (_blob.id -> _blob)
case _ => _passthroughFields += _root_.scala.Tuple2(_blob.id, _blob)
}
new AnotherException(
errorCode,
Expand Down Expand Up @@ -317,15 +306,14 @@ class AnotherException(

override def canEqual(other: Any): Boolean = other.isInstanceOf[AnotherException]

private def _equals(x: AnotherException, y: AnotherException): Boolean =
x.productArity == y.productArity &&
x.productIterator.sameElements(y.productIterator) &&
x.flags == y.flags &&
x._passthroughFields == y._passthroughFields
private[this] def _equals(other: AnotherException): Boolean =
this.productArity == other.productArity &&
this.productIterator.sameElements(other.productIterator) &&
this.flags == other.flags &&
this._passthroughFields == other._passthroughFields

override def equals(other: Any): Boolean =
canEqual(other) &&
_equals(this, other.asInstanceOf[AnotherException])
canEqual(other) && _equals(other.asInstanceOf[AnotherException])

override def hashCode: Int = {
31 * _root_.scala.runtime.ScalaRunTime._hashCode(this) +
Expand All @@ -334,14 +322,6 @@ class AnotherException(

override def toString: String = _root_.scala.runtime.ScalaRunTime._toString(this)


override def productArity: Int = 1

override def productElement(n: Int): Any = n match {
case 0 => this.errorCode
case _ => throw new IndexOutOfBoundsException(n.toString)
}

override def productPrefix: String = "AnotherException"

def _codec: ValidatingThriftStructCodec3[AnotherException] = AnotherException
Expand All @@ -359,18 +339,19 @@ class AnotherException(
private[thriftscala] class AnotherExceptionStructBuilder(instance: _root_.scala.Option[AnotherException], fieldTypes: IndexedSeq[ClassTag[_]])
extends StructBuilder[AnotherException](fieldTypes) {

def build(): AnotherException = instance match {
case _root_.scala.Some(i) =>
def build(): AnotherException = {
val _fieldArray = fieldArray // shadow variable
if (instance.isDefined) {
val instanceValue = instance.get
AnotherException(
(if (fieldArray(0) == null) i.errorCode else fieldArray(0)).asInstanceOf[Int]
if (_fieldArray(0) == null) instanceValue.errorCode else _fieldArray(0).asInstanceOf[Int]
)
} else {
if (genericArrayOps(_fieldArray).contains(null)) throw new InvalidFieldsException(structBuildError("AnotherException"))
AnotherException(
_fieldArray(0).asInstanceOf[Int]
)
case _root_.scala.None =>
if (fieldArray.contains(null)) throw new InvalidFieldsException(structBuildError("AnotherException"))
else {
AnotherException(
fieldArray(0).asInstanceOf[Int]
)
}
}
}
}

Loading

0 comments on commit 2189d28

Please sign in to comment.