Opinion Neutralization Harms Performance and Not Necessary

[deusaquilus]

Version: (e.g. 3.5.2)
Module: (e.g. quill-core)
Database: (e.g. all)

Ast Opinions have been introduced to Quill in order to allow a secondary channel of propagation of information throughout the AST. In order for these not be considered by BetaReduction (i.e. since it uses Map which relies on hashcode/equals methods of case classes), I have added a mechanism that "neutralizes" these opinions however, as it turns out this mechianism is quite slow because it requries recursive traverse for the sub-ast for every element that beta-reduction must check for replacements. For this reason, I have decided to instead create separate case classes for Ident, Property, and other elements with opinions which are just used for purposes of equals and hashcode functions (they are called IdentId, PropertyId etc....).

[mjsmith707]

I see you removed Replacements in #1911 so this is a bit late now but I made some experimental changes to speed this up after our chat. The replacements change seemed to work very well:
mjsmith707@26e4652 and I was going to submit it upstream. I can't verify it doesn't break anything but I observed no effects in our codebase.

After that I did some probably very controversial changes to try and speed up the AST walking:
mjsmith707@c9c73ec#diff-71371a59bb216cadcfcc0425b09e41bbR32 Namely changing it to a switchtable instead of a pattern match + unapply. This had a fairly substantial speedup but is a big drop in readability sadly (scala requires those ints to be constant, can't use a final val).

The sum of those changes took a dynamicQuery call I was building from >35ms to around 12ms per call (only microseconds spent in JDBC and Postgres) in a fairly unscientific benchmark (just running it in an infinite loop). Almost all of that speedup was Replacements + the main apply in StatelessTransformer. It may be better to switch to a visitor pattern instead but that is of course, a massive change.

I'm planning on following up with the performance examination after we address some other issues in our codebase (lots of runSyncUnsafe+Await(future) usage). Let me know if you want more profiling information and thanks again.

[deusaquilus]

@mjsmith707 You changes here make sense but for the future maintainability of Quill, I can't do them. I am really hoping, however, that with ScalaFix something like this can be done in a compile-phase. Please have a look at this video by Dimitry Petrashko for some inspiration, particularly where he talks about library-specific rewrite rules (I think that's the right time-code).
https://www.youtube.com/watch?v=Xv2Z4ZdInXc&feature=youtu.be&t=1624

One other important thing to mention is that in #1911 I have added Quill Applications Types which will undoubtedly introduce new performance issues. Specifically, for things like renaming properties, I have decided to group paths and use hashing to detect what should be renamed as well as hashtable iteration to match on Quat field comparisons. This is optimized for larger records that have many renames (e.g. 100+), for smaller records however it is possible that a linear search is superior. There will be additional profiling that has to be done. Please let me know your findings.

[deusaquilus]

@mjsmith707 Here's an example of how this could be done with ScalaFix. Firstly I would add some kind of identity-unapply to mark a beta reduction clause as 'pure' i.e. it uses only the case-class-companion unapply method:

object Pure {
  def unapply[T](t: T) = Some(t)
}

This can then be used to mark clauses which we specifically know do not use unapply functions from custom things. For starters, we could even make the assumption that that clauses marked this way are completely flat.

For example, something like this:

// StatelessTransformer.scala
def unapply(q: Query): Option[Query] =
  q match {
    case Map(a, b, c)       => apply(a, c)(Map(_, b, _))
    case FlatMap(a, b, c)   => apply(a, c)(FlatMap(_, b, _))
    ...
  }

Could become this:

// StatelessTransformer.scala
def unapply(q: Query): Option[Query] =
  q match {
    case Pure(Map(a, b, c))       => apply(a, c)(Map(_, b, _))
    case Pure(FlatMap(a, b, c))   => apply(a, c)(FlatMap(_, b, _))
    ...
  }

Then, a ScalaFix rule would identify these clauses and knowingly convert the code to this:

// StatelessTransformer.scala
(q.queryTag: @switch) match {
  case 21 => None
  case 23 =>
    val e = q.asInstanceOf[Map]; apply(e.query, e.body)(Map(_, e.alias, _))
  case 24 =>
    val e = q.asInstanceOf[FlatMap]; apply(e.query, e.body)(FlatMap(_, e.alias, _))

ScalaFix would have to know some other things like the id-integers of the different members of the Quill AST, again, I don't mind adding annotations to let ScalaFix know that these things should be generated.

For example:

// Ast.scala
@pureId // I.e. generate a final val Map: Byte = {pick some number from a sequence}
case class Map(query: Ast, alias: Ident, body: Ast) extends Query { def quat = body.quat }

@pureId // I.e. generate a final val Map: Byte = {pick some number from a sequence}
case class FlatMap(query: Ast, alias: Ident, body: Ast) extends Query { def quat = body.quat }

The nice thing about this kind of code is that it will work whether the Scala-Fix optimization rules are applied or not. That is the sweet-spot that I would like to be at regarding the optimization vs maintainability tradeoff.

[mjsmith707]

@deusaquilus Yes of course the experimental AST stuff was never intended to be merged upstream :). I thought the Replacements stuff probably could be but that has now been removed and is obsolete.

ScalaFix does look like an interesting way to maybe solve that issue. I'll hold off on looking further into it until a new release is cut with #1911 and I get a chance to re-profile against it.

[deusaquilus]

Closing this for now since #1911 largely remove opinion-neutralization.

@mjsmith707 If you'd like, please open a Issue to explore getting switch-optimization of Quill transformations via ScalaFix rewrites.

[mjsmith707]

@deusaquilus I think we're still very much interested but are working through other issues first before we get back around to this. We've also filed #1846 awhile back and may focus on seeing what it takes to fix that first.