Symbols: share the set of Attachments

[diesalbla]

In some profiles, we noticed that the call to clone.updateAttachment was generating up to 10 MiB of lambda objects. Since this seems to be non-reentrant, we can save those allocations by using a single Function1 object with a mutable state.

[retronym]

I'd prefer to take this in a slightly different direction which would avoid allocation of intermediate NonEmptyAttachments and immutable.Set instances when cloning symbols that contain non-empty attachment sets.

diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
index ce0b975f7c..b834232eba 100644
--- a/src/reflect/scala/reflect/internal/Symbols.scala
+++ b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -2025,7 +2025,8 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
           setInfo (this.info cloneInfo clone)
           setAnnotations this.annotations
       )
-      this.attachments.all.foreach(clone.updateAttachment)
+      assert(clone.attachments.isEmpty)
+      clone.setAttachments(this.attachments.cloneAttachments)
       if (clone.thisSym != clone)
         clone.typeOfThis = (clone.typeOfThis cloneInfo clone)

diff --git a/src/reflect/scala/reflect/macros/Attachments.scala b/src/reflect/scala/reflect/macros/Attachments.scala
index 157d142f2e..ce87107e89 100644
--- a/src/reflect/scala/reflect/macros/Attachments.scala
+++ b/src/reflect/scala/reflect/macros/Attachments.scala
@@ -76,6 +76,7 @@ abstract class Attachments { self =>
   }

   def isEmpty: Boolean = true
+  def cloneAttachments: Attachments { type Pos = self.Pos } = this
 }

 private object Attachments {
@@ -90,4 +91,5 @@ private final class NonemptyAttachments[P >: Null](override val pos: P, override
   type Pos = P
   def withPos(newPos: Pos) = new NonemptyAttachments(newPos, all)
   override def isEmpty: Boolean = false
+  override def cloneAttachments: Attachments { type Pos = P } = new NonemptyAttachments[P](pos, all)
 }

[retronym]

Consider alternative of reusing the underlying immutable.Set when cloning a symbol with non-empty attachments.

[diesalbla]

Just for my information: are those NonEmptyAttachments and immutable.Set instances created by the Lambda? What I could read on the profile was 10 MiB of Lambda objects, which is why it made sense to reuse them with a mutable instances.

Are those changes you mentioned complementary or opposite to extracting the Lambda itself?

[retronym]

The common case is that the attachment set is empty. In that case, the only allocation here is the Lambda.

But for symbols with a non-empty set of attachments, the current means of cloning is inefficient -- it creates a copy of the the underlying set element by element (discarding the wrapper class NonEmptyAttachments each step!).

But the underlying Set is immutable, so could just be structurally shared as the basis of the cloned symbols attachment set.

When we refactor the code to do this, we no longer call foreach so we don't need the lambda at all.

[retronym]

For future reference, I realised that this was defined at the wrong level -- it should have been a member of Symbols not of Symbol.

[diesalbla]

/rebuild

[retronym]

Needs a binary compatibility exception: ProblemFilters.exclude[DirectMissingMethodProblem]("scala.reflect.macros.Attachments.cloneAttachments") in build.sbt

[retronym]

/synch

[retronym]

/nothingtoseehere