Capture TTID timestamp at draw time

onDraw() is the event boundary for TTID, but this refactor now reads timeProviderNs() inside reportFirstFrame() after the callback has been queued, so the recorded timestamp can drift later than the actual first draw when the main thread is busy. That inflates durationNs and makes startup metrics less accurate. Capture the timestamp in onFirstDraw() and pass that fixed value into the queued runnable (as the previous implementation did).

Useful? React with 👍 / 👎.

This is a good comment. However this change is on purpose. In the original RFC I wanted to do exactly this (read here).

But at some point (I don't remember when) apparently I made a mistake and instead of using the time of when the Runnable is executed, I used the time of when onDraw was called.

🤔 Just to understand, why don't we want to use the time when onDraw is called?

It is explained in the link to the RFC that I mentioned above. In short - it is more precise.

I do it individually in each test, not in one place, because:

1. If we do it here, some strange memory leak is detected. It is 100% not a problem in our SDK, but rather in our tests. I haven't managed to find a solution to this.
2. We can't use the test rule, because it checks the leaks after Datadog.stop is called here which clears all features including RumFeature that itself clears this. It allows interstitial Activity to be garbage collected and the leak isn't detected.

just curious: Won't it be enough just to keep identityHashCode as a key? Don't we risk to have a minor leak here by keeping the key longer than needed (it will be GC-ed eventually since it is a WeakHashMap)?

But I guess onActivityDestroyed is called all the time during the normal app lifecycle.

My current understanding is that identityHashCode should not be used as a key in hash maps.

identityHashCode returns an int. So it can represent only 2^32 distinct values. This means that for 100 000 items the probability of at least one collision is approximately 68% (see this and to calculate this). We will not have 100 000 Activities - yes, but some other things (some kind of spans maybe) - why not. So I don't think it is a good practice in general.

Also see this.

But I guess onActivityDestroyed is called all the time during the normal app lifecycle.

yes, exactly. Here I use WeakHashMap just as a precaution against some kind of mistake. It could be replaced by a normal Map.

https://pl.kotl.in/Ebe5fCsD4 https://pl.kotl.in/mp61XH2LK - sample to reproduce a collision.

That is true. My idea is that since it is just used as a key in the map, where lookup is done by first checking hashCode for a bucket and then reference comparison, then maybe we can capture some identity property of Activity instead and use it.

But as long as it is guaranteed to do a cleanup in onActivityDestroyed - it is okay.

Looking at the reproducer: I think it is expected behavior, because obj can be GC-ed on each loop end, so system can give the int which was already seen before, but it is not attached to any object (since previous owner was already GC-ed).

yes, you are right. This one is better https://pl.kotl.in/mp61XH2LK

Also the interesting thing about this https://pl.kotl.in/mp61XH2LK is that every time you run it it finds the collision at exactly the same iteration 105668 - which proves the point here that some kind of pseudo-random number generator is involved in generating the identity hash code.

🤔 Just to understand, why don't we want to use the time when onDraw is called?