eidos icon indicating copy to clipboard operation
eidos copied to clipboard

Allocate only one Label per tag type, and only one default Label.

Open thatfulvioguy opened this issue 6 years ago • 5 comments

I was looking at potential memory optimisations in an application and noticed that currently, a new Label is created for each Id. This can be avoided by:

  • Having Label.default always return an existing single default Label value
  • Defining the implicit on CustomLabel as a val instead
  • Defining the implicit on MakeLabel as a val, ~though this comes at the cost of a self type error when attempting to use a non-case object as a tag.~

thatfulvioguy avatar Sep 19 '19 17:09 thatfulvioguy

Did you notice this as an actual bottleneck or just a nice to have? Because if it's a nice to have, I wouldn't want to sacrifice the error message, it can be pretty obscure otherwise.

SystemFw avatar Sep 19 '19 22:09 SystemFw

Yeah, it's a nice-to-have. However, I've just made it work using a Manifest instead:

private def nameOf[A](implicit m: Manifest[A]) = {
  val typeName = m.toString
  typeName.substring(0, typeName.lastIndexOf(".type"))
}

trait MakeLabel { self =>
  // See eidos.id.Format.UUID for an explanation of this
  // format: off
  final def `"In Eidos, you can only extend one of MakeLabel or CustomLabel"`
      : LabelDefinitionConflict = null

  implicit final val l: Label[this.type] = new Label[this.type] {
    val label: String = nameOf[self.type]
  }
}

With that, it should be possible to remove the case requirement entirely. Is that something you'd like?

thatfulvioguy avatar Sep 20 '19 10:09 thatfulvioguy

Manifest uses reflection though, which means you are getting back a possibly larger perf penalty for that, I think

SystemFw avatar Sep 20 '19 10:09 SystemFw

The manifest is only created once when the tag object and its label are constructed

thatfulvioguy avatar Sep 20 '19 11:09 thatfulvioguy

That's the Manifest version pushed. Should I continue with relaxing the case object requirement?

thatfulvioguy avatar Sep 20 '19 13:09 thatfulvioguy