Tweak reusable instances [ci: last-only] #8871
Conversation
|
I forgot to create as draft and also ci last-only. There are 360 reusable instances in the scala build, mostly SymbolLookup, with the cache hit 400K times for SymbolLookup and 3.5M for FindMember. Just a handful of reusages are deeper than 4, so the array is initially 4. SymbolLookup had 1 instance go to 9 (and reused a couple of times). |
som-snytt
changed the title
We modify ReusableInstance, so that instead of storing a single instance in a mutable variable, we store a mutable stack (an ArrayBuffer) of them. Currently, when a reentrant call to the `using` method of ReusableInstance occurs, a new object is allocated and discarded. Using a stack allows us to keep any number of instances around, so that the maximum number of allocated instances is the maximum reentrant (recursion) stack depth.
Use factory methods, avoid needless allocations.
|
Rebased with the cherry-picked commit. |
| private var taken = false | ||
| final class ReusableInstance[T <: AnyRef] private (make: => T, enabled: Boolean) { | ||
| private[this] val cache = if (enabled) new ArrayBuffer[T](ReusableInstance.InitialSize).tap(_.addOne(make)) else null | ||
| private[this] var taken = 0 |
There was a problem hiding this comment.
I was going to complain that this would stymie inlining, but lo:
// make not private symbol accessed from inner classes, as well as
// symbols accessed from @inline methods
//
// See scala/bug#6552 for an example of why `sym.owner.enclMethod hasAnnotation ScalaInlineClass`
// is not suitable; if we make a method-local class non-private, it mangles outer pointer names.
def enclMethodIsInline = closestEnclMethod(currentOwner) hasAnnotation ScalaInlineClass
// scala/bug#8710 The extension method condition reflects our knowledge that a call to `new Meter(12).privateMethod`
// with later be rewritten (in erasure) to `Meter.privateMethod$extension(12)`.
if ((currentClass != sym.owner || enclMethodIsInline) && !sym.isMethodWithExtension)
sym.makeNotPrivate(sym.owner)
|
If you stand diesalbla atop hrhino, they come up to retronym's shoulders, so I'm prepared to merge. Especially now that retronym is actually reusing the data reader. |
|
LGTM. ( that just autocorrected to “litmus”, which I liked. Is that the name of a test framework yet?) I might circle back to this to make the maximum depth configurable. The classfile reader is pretty big and rarely reentrant, so a limit of 1 would make sense. |
SethTisue
added
the
performance
Avoid some allocations if not enabled; and use factory methods.
Follow-up to the companion PR.