Use ClassValue to store ThreadLocal call depth

[richardstartin]

@bantonsson can you try this?

[bantonsson]

Since I'm mostly looking at the memory, it obviously shaves off the last % of the HashMap$Node, and I can't see anything else popping up. 👍

[richardstartin]

I am under the impression that ClassValue.get is more efficient than HashMap.get(Class<?>) but haven't measured it, and in most instrumentations we would expect there to be fewer classes with tracked depths than the default size of a HashMap, so I expect this should use less heap. In any case, the change has the commit from #1527 so will apply cleanly if we want to do this later and merge that now.

[tylerbenson]

Crazy idea, but instead of removing, should we just reset the depth to 0?

[richardstartin]

@bantonsson what do you think? On the one hand, this is a recipe for a memory leak with non cached threads, on the other it's a parsimonious approach when threads are cached.

[bantonsson]

I actually think that it's fine to leave the Depth in there. I mean if people are creating threads and they don't run to completion, they have other issues.

[richardstartin]

Done

[bantonsson]

I agree that this is probably better than the HashMap version. I think this should go in.

[tylerbenson]

Let's merge both PRs since this one builds off the other one. (But merge this one last.)

[richardstartin]

@tyler we can just merge this because it has @bantonsson's commit

[bantonsson]

👍

[dougqh]

I am under the impression that ClassValue.get is more efficient than HashMap.get(Class<?>) but haven't measured it, and in most instrumentations we would expect there to be fewer classes with tracked depths than the default size of a HashMap, so I expect this should use less heap. In any case, the change has the commit from #1527 so will apply cleanly if we want to do this later and merge that now.

I haven't used ClassValue myself previously. But from Googling and reviewing the ClassValue, it does look promising. Also nicely handles class unloading from what I've read. Maybe, we should be using it more broadly. Although in many places, we really need the equivalent of a ClassLoaderValue.