refactor: cache constant tags - #915
Conversation
847d554 to
bb17d8e
Compare
bb17d8e to
e0d59c3
Compare
| ): | ||
| # Build the combined tag string from user tags + cached constant tags | ||
| if tags: | ||
| user_tags_str = ",".join(normalize_tags(tags)) |
There was a problem hiding this comment.
Would it make sense to append user tags and comma separators as individual elements to the parts array instead of building a temporary string?
If we do, can the same be done for constant_tags as well (to avoid the need to build and cache the string)?
There was a problem hiding this comment.
Would it make sense to append user tags and comma separators as individual elements to the parts array instead of building a temporary string?
This I would say we can, yes.
If we do, can the same be done for constant_tags as well (to avoid the need to build and cache the string)?
This I wouldn't be so sure, my fear would be that it would make performance worse (better than before, but not as good as this version), as you'd still be building the very long string partly from scratch at the end when you join the parts.
I can give it a go and check what kinds of results I get from it, though.
There was a problem hiding this comment.
I tried this and it performs worse than the other version (the one currently on the branch), so I'd advocate against this if that is fine by you.
There was a problem hiding this comment.
Just curious, how much worse? Is it better or worse than the unoptimized version?
There was a problem hiding this comment.
It depends :) I ran the same benchmark and this is the result (numbers are ops/s):
- 5 const + 3 user tags: master 2.2M, optimised: 1.7M (-21.3%)
- 5 const, no user tags: master 2.315M, optimised: 2.6M (+11.9%)
- 3 user, no const tags: master 2.7M, optimised: 2.5M (-4.5%)
- No tags, master: 4.8M, optimised: 5.5M (+15.1%)
- 10 const + 2 user tags, master: 2M, optimised: 1.5M (-28.8%)
|
@vickenty could you give another look please? |
StephenWakely
left a comment
StephenWakely
left a comment
There was a problem hiding this comment.
There's a couple of places where constant tags isn't thread safe. I don't think this was introduced by this PR, but I think it would be worth fixing. Would it be possible to do? Does it affect the benchmarks in any way?
|
|
||
| def _add_constant_tags(self, tags): | ||
| if self.constant_tags: | ||
| if self._constant_tags: |
There was a problem hiding this comment.
This needs to lock self._config_lock.
There was a problem hiding this comment.
Added this -- I don't think this should have any impact on the benchmark results as this is not on the hot path.
| parts.append(text(sample_rate)) | ||
|
|
||
| if tags: | ||
| if tags or self._constant_tags_str: |
There was a problem hiding this comment.
This needs to lock self._config_lock.
There was a problem hiding this comment.
I don't think it does if we capture it first?
Like instead of doing if tags or self._constant_tags_str, we do
constant_tags_str = self._constant_tags_str
if tags or constant_tags_str:
# use constant_tags_str, not self._constant_tags_strWhat do you think? Running benchmark shows acquiring the lock here is somewhat costly.
The assignment I do in the proposed code is thread-safe in Python so while it may differ slightly from the-same-code-but-with-a-lock, but at least doing the capture instead of re-reading the attribute avoids the TOCTOU race.
There was a problem hiding this comment.
Ok, yes good call. The lock isn't needed. I hadn't realised that these reads are atomic in CPython. I don't think capturing it first is necessary.
There was a problem hiding this comment.
I do believe we need capture if we want to be consistent between the conditions and usage, right?
There was a problem hiding this comment.
Oh, yes true, it's used afterwards.
a7b5f48 to
0913ea9
Compare
88fcd43 to
6c452c7
Compare
179b1fb to
1c9a718
Compare
4 participants
What is this PR?
This PR updates the way we handle constant tags to improve the overall performance of reporting metrics.
Previously, we would compute the string of user tags + constant tags at every
reportcall; now we insteadcache the constant tags part and only compute the string for user tags and the sum of both.
When the
DogStatsDinstance has even a few constant tags, this can make a significant difference in howmany allocations and processing we have to do.
In practice
constant_tagsis now a@propertywith a setter that pre-computes_constant_tags_str(the normalised string)_reportdoes not call_add_constant_tagsanymore for metrics._add_constant_tagsis kept foreventandservice_checkpaths, but updated to useself._constant_tags.The gains
_add_constant_tagslist concatenation — previously allocated a newtags + self.constant_tagslist on every call; this is gone.normalize_tagsran regex substitution over every tag on every call. Now constant tags are normalised once at init.",".join(...)ran over all tags every call. Now constant tags are pre-joined; only user tags need joining at call time.In order to maintain existing behaviour when using
statsd.constant_tags.<mutating_method>(e.g.clearorappend), I had to add aTagListclass that is a very basic subclass oflistand that updates the cached value when it is mutated.The alternative would have been to make it immutable (e.g. return a tuple) but this would have broken potential existing usages, which doesn't sound worth it.
Benchmark results
Setup: 5M
statsd.increment()calls per case (with buffering enabled).The baseline is the code before my changes, optimised adds the constant tags cache.
The win scales with the number of constant tags and to a lower extent user tags.