Skip to content
This repository has been archived by the owner on Dec 29, 2022. It is now read-only.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thatfulvioguy
Copy link

@thatfulvioguy thatfulvioguy commented Sep 19, 2019

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.

@SystemFw
Copy link
Owner

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.

@thatfulvioguy
Copy link
Author

thatfulvioguy commented Sep 20, 2019

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?

@SystemFw
Copy link
Owner

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

@thatfulvioguy
Copy link
Author

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

@thatfulvioguy
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

2 participants