Skip to content
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.

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

Annotations.labelOf is catastrophically slow #288

Closed
timw opened this issue Oct 17, 2019 · 4 comments · Fixed by #289
Closed

Annotations.labelOf is catastrophically slow #288

timw opened this issue Oct 17, 2019 · 4 comments · Fixed by #289

Comments

@timw
Copy link
Collaborator

timw commented Oct 17, 2019

#253 introduced in Annotations.labelOf[T] an implementation of extracting the @label value that worked for non literal values (e.g. @label(SomeClass.LABEL) as well as literal values (e.g. @label("vert")).

This implementation, unfortunately, is terribly slow (to the point I noticed a single invocation of the method slowed a unit test by over a second, which got me to investigating).

The labelOf implementation uses Toolbox.eval, which invokes the back-end of the Scala compiler.
This implementation averages about 65ms per invocation on my dev system (an i7 MBP).

An implementation that only uses the introspection APIs is 1000 times quicker (something like 60 micro seconds):

  def fastLabelOf[T <: Product: WeakTypeTag]: Option[String] = {
    weakTypeOf[T].typeSymbol.asClass.annotations
      .find(_.tree.tpe =:= typeOf[label])
      .map( _.tree.children.tail.head match { case Literal(Constant(id: String)) => id })
  }

This is in essence what the Marshallable materialization macro and the original code does.
The problem is obviously that this regresses the ability to retrieve a non literal value.

A macro based implementation is 1,000,000 times quicker:

  def labelOf[T]: Option[label] = macro labelOfImpl[T]

  def labelOfImpl[T: c.WeakTypeTag](c: Context): c.Expr[Option[label]] = {
    import c.universe._
    val tpe = weakTypeOf[T]

    val label = tpe.typeSymbol.asClass.annotations
      .find(_.tree.tpe =:= weakTypeOf[label])
      .map { annotation => q"""Some(new label(${annotation.tree.children.tail.head}))"""}
      .getOrElse(q"None")
    c.Expr[Option[label]](label)
  }

(This is keeping the labelOf[T]: Option[label] contract - in actual use, we only need the String value).

Unfortunately this has another problem, in that it needs a WeakTypeTag of a real class type, which can only be produced at the time of invocation.
e.g. Calling Annotations.labelOf[MyType] will work, but not Annotations.labelOf[T] as we see in GremlinScala.hasLabel[CC]. The WeakTypeTag that is passed through hasLabel isn't compatible with the implicit macro context WeakTypeTag required by the macro.

At this point I'm not sure what the best resolution is (otherwise I'd have PRed this):

  • There may be some Scala macro fu I don't know of to adapt the WeakTypeTag so it can be invoked indirectly through methods like hasLabel.
  • Alternatively the API style for case class use in GremlinScala and ScalaGraph could change to take an implicit LabelExtractor implementation materialized by the macro (i.e. instead of the implicit WeakTypeTag evidence provided by the type context bound now). I have this prototyped, and it works.

I'm happy to put together a PR if there's a consensus on a solution, but I can't think of anything at the moment that doesn't break API and/or ABI.

@mpollmeier
Copy link
Owner

Wow, that's indeed catastrophically slow, thank you for investigating!
I'm leaning towards reverting the original PR. @jeremysears do you have any thoughts on this?

@jeremysears
Copy link
Collaborator

jeremysears commented Oct 18, 2019

We now generate all of the case classes that represent elements in our schema (as well as our ShiftLeftSecurity/tinkergraph-gremlin vertex/edge implementations and much of the machinery that's needed for all of that such as files enumerating constants). The constants we keep for labels happen to be resolved via labelOf instead of used in the labelOf annotation. Here is a example CC we use in test:

@label("example.BogusNode1")
@relations(
  relation(EdgeLabel.BogusEdge2, MULTI, DIRECTED, classOf[com.example.model.test.BogusNode1]),
  relation(EdgeLabel.BogusEdge1, MULTI, DIRECTED, classOf[com.example.model.test.BogusNode1], classOf[com.example.model.test.bogus.BogusNode2]),
  relation(EdgeLabel.EdgeWithNoProperties, MULTI, DIRECTED, classOf[com.example.model.test.BogusNode1], classOf[com.example.model.test.bogus.BogusNode2])
)
case class BogusNode1(
    @id id: Option[Long] = None,
    @underlying underlying: Option[Vertex] = None,
    @key var vertexProperty2: Option[String],
    @key var vertexProperty1: String,
    @key var vertexListProperty: List[String],
    @key var vertexSetProperty: Set[String])
  extends ModelVertex 

object BogusNode1 {
  /** The vertex label for graph elements of this type */
  val Label: String = labelOf[BogusNode1].get.label
  /** A comment about vertexProperty2 */
  val VertexProperty2: Key[String] = Keys.VertexProperty2
  /** A comment about vertexProperty1 */
  val VertexProperty1: Key[String] = Keys.VertexProperty1
  /** A comment about vertexListProperty */
  val VertexListProperty: Key[String] = Keys.VertexListProperty
  /** A comment about vertexSetProperty */
  val VertexSetProperty: Key[String] = Keys.VertexSetProperty
}

Long story short, this no longer affects. It may affect others, so it's probably worth leaving a release note if you change the behavior if you don't go with one of @timw 's other suggestions.

@jeremysears
Copy link
Collaborator

Regarding @timw 's analysis, everything he mentions seems sound.

@mpollmeier
Copy link
Owner

revert PR: #289

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants