Refactor jank to use events #1175
Conversation
… JankReporter interface
bidetofevil
left a comment
bidetofevil
left a comment
There was a problem hiding this comment.
LGTM. Nothing blocking. The leak in the case of a removal of the SlowRenderListener before clean up should be straightforward, but it's theoretical and if you don't think it'll happen, don't worry about it.
| private val frameMetricsHandler: Handler, | ||
| private val pollInterval: Duration, | ||
| ) : DefaultingActivityLifecycleCallbacks { | ||
| private val activities: ConcurrentMap<Activity, PerActivityListener> = ConcurrentHashMap() |
There was a problem hiding this comment.
In case this callback is removed before it has a chance to process the deregistration of the listener for a particular activity, I'd use a WeakHashMap and WeakReference keys here as we don't want this to be the only reference preventing the GC of a dead Activity.
There was a problem hiding this comment.
I agree we should avoid keeping hard references to Activities.
There was a problem hiding this comment.
Hmmm, you really got me thinking about this. 😅 It would be quite undesirable indeed to leak Activity references. I've looked at it from a few different angles and have just concluded that it's complicated.
I'm not sure how it could happen, other than via SlowRenderingInstrumentation.uninstall() which both unregisters the listener, shuts down the SlowRenderListener which removes all the frame listeners and clears the activity map, and then it nulls its own reference...which should allow it to be GCd anyway. Agreed?
So if I reason through that I think we're already covered...unless you have another way that this listener might get deregistered?
There was a problem hiding this comment.
Because it's been this way for a LONG time, and In the interest of moving forward, I'll open an issue for this and we can tackle it in a follow-up effort.
There was a problem hiding this comment.
Ok #1192
I think we might also have a problem with referencing the wrong Window instance if/when an Activity changes it. Please have a look.
There was a problem hiding this comment.
Got it. I didn't realize we've been doing it this way all this time. I think we can move forward with this for the purposes of migrating to the new spec, though we should definitely try to address that new issue asap.
4 participants
Resolves #904.
Also attempts to address the prototype implementation in open-telemetry/semantic-conventions#2552.
This was done in a way that allows us to continue emitting the zero-duration span, but to mark it as deprecated for a release and remove it in the next release.