-
Notifications
You must be signed in to change notification settings - Fork 229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance improvements around dictionary locking #54
Comments
This might work in theory... I see few practical problems:
First two problems are probably easily solvable (e.g. by doing everything in one iteration). The other two have solution too... Locks can be implemented on top of dict. Atomicity can also be ensured by using some indirection and dict. The only question is, how much slower it would be and how much of a trouble is that for users. Anyway, I would be happy to test it, if you have time to code it. |
That's a good idea, thanks for suggesting it. As you mentioned, actually merging results back is the tricky bit; while counters can just be incremented atomically, for gauges there will be a race between the thread copying old data from A to B and other threads changing gauge values. One way to avoid this is to actually have three dicts - two for counters, and a single one for gauges (that will not be swapped). I suspect most users will have significantly more counters than gauges (especially given that histograms are implemented with counters), so this will have the desired performance benefit. Note, that shared dictionaries are defined outside of the library, so this will not be backwards compatible; users will need to provide several dicts now instead of one. Finally, we'll need to implement locking to make sure We'd probably also want to have some sort of performance benchmark added first, and then use it to validate performance improvements from this change. In general, this is not something I am planning to implement myself, but will be happy to collaborate on design and review the code. |
Oh nice, @dolik-rce beat me to the first response with a very similar set of concerns :) Thanks! I agree that switching between dicts atomically is another tricky bit about this. My previous thinking about resolving the performance issue was to add support for other key/value stores (besides builtin dict) that provide a similar API - for example, an external Redis instance. I suspect this actually might be easier to implement correctly than doing the double dictionary dance. |
Thanks for reading and writing back your concerns @knyar @dolik-rce, that's really helpful. We're thinking about using this library and just wanted to make sure that, if we ever hit a performance wall, there are ways around it. 🙌 I won't be working on this any time soon, unless it affects us, which it hasn't yet. So I'm gonna go ahead and close this one. Thanks again! |
Well, I actually use a patched version of this module using yet another approach to avoid |
That's also a nice idea, @dolik-rce. I think you don't need locking if you can accept the same key being stored multiple times when several threads are racing to create a new metric, and then ignore duplicate names while reading the keys. I will be curious to see your current implementation. The way I would probably do it is:
Then, while writing a value:
If multiple threads race between steps 1 & 3, the same metric name will be written with several indexes. While collecting metrics:
What do you think? |
That is actually almost exactly the algorithm I use, good to see you arrived to same conclusions :-) The only part I didn't think of is resolving the race condition when collecting the metrics (I actually found there is one just last week, when looking at the code because of this issue). Additionally, I used a table to keep all the metrics keys in each worker. This table can be quickly updated by iterating only over newer indexes (since we only add metrics, never modify or remove) and I used it to implement nice and fast I think it could actually be even slightly simpler if you omit the __metric_last. You don't actually need it, since you can iterate over the |
FYI, there have been some recent performance improvements for counter metrics that should decrease lock contention: b7c8feb...d8c2813 Counters are now incremented independently by each worker process, with deltas flushed regularly to the shared dictionary asynchronously. Changes are now in Even though implementation is different to the one described in this issue, I think it largely addresses performance concerns. I will close this issue now, but please feel free to reopen (or create a new issue or a PR) if you would like to suggest further improvements. |
I was just reading the performance issue with locking and I'd like to suggest a possible solution: double buffering.
In a nutshell, write everything to one shared dict (A). Then, when it's time to
collect
, we redirect all writes to a second dict (B) so they don't block.collect
is done only from A's data. For the next scrape, we treat A as B and vice versa.After
collect
finishes we'll have to:(2) is where it can get a bit hairy but I think it's doable (maybe using CRDTs)?
The text was updated successfully, but these errors were encountered: