stats: keep a set of ref-counted parent histograms in ThreadLocalStore so that two with the same name map to the same histogram object.

[jmarantz]

Commit Message: Creates a storage model for thread-local histograms enabling continuity of data across scope reloads. Previously, whenever a Scope was re-created, the counters, gauges, and text-readouts of the same names would retain their previous values. However, fresh histograms were created on every scope reload, and stats dumps would include duplicate histograms with the same name.

This change adds an analogous name-based set of histograms, held in ThreadLocalStore, so that we have a single histogram representing all its generations of data. This is somewhat more complex than for the other stats, since there were thread-local buffers, which previously were owned by TlsScope and needed to be made independent. So this introduces a new tls histogram map in the tls-cache to maintain this.

This should help unblock #12241.

Additional Description:
Risk Level: high (not clear whether this is enough testing of histogram usage)
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes: #3098

[jmarantz]

This may help unblock #12241

[jmarantz]

We'll see if this flies through CI but I think this is ready for review

[ggreenway]

I don't understand this code well enough to effectively review most of it. @mattklein123 can you take a look?

[mattklein123]

I should have definitely not tackled this at the end of the day on a Friday. ;)

Awesome to see this long standing issue fixed. At a high level this makes sense to me though I had to spend quite a bit of time with it to page things in. Beyond my comments below a few high level comments:

1. Can we update stats.md as part of this PR?
2. I think it would be good to have more tests. There are lots of very specific threading behaviors here that I think we can test with the synchronizer?

Anyway thanks for working on this!

/wait

[jmarantz]

I'll look at indirect testing of the cleaning of TLS histograms, as well as testing with real threads.

There are a bunch of other tests in the same file with real threads, so it shouldn't be too hard. I don't know if I need to use the synchronizer unless I specifically want to test a race I'm suspicious of. Is there a particular race you think I should focus on?

[jmarantz]

OK I've got a framework for a real-threads histogram test, which I refactored from an existing test that used real threads.

I think there's probably more you had in mind and I should take another crack at some tests myself tomorrow, and I still need to update stats.md.

[mattklein123]

Thanks in general this LGTM. I don't specifically have ideas for tests, but I think the more you can add the better as this code is so complicated and hard to reason about. Also, can you update stats.md?

/wait

[jmarantz]

I think this is ready for review; I basically duplicated the histogram "overlap" test but with real threads, and added more comments around the threading model, and updated stats.md. Note that nothing that was in stats.md was actually wrong, but I added detail about the tls structures.

[jmarantz]

note: #12035 removed this commented-out section, probably due to a git merge conflict error.

This PR actually does reduce memory usage due to reducing the number of map instances, and I put the updated values into the comment log, but I re-tested the flakiness and still measured 3 flakes out of 100 runs ,so I commented out the tests again.

[jmarantz]

I didn't add more 'synchronizer' tests. The locking structure is identical to that of counters/gauges for which there are synchronizer tests. Happy to add that if you like.

[mattklein123]

Thanks!

approved by mklein