ethersphere
chore: reduce locking extent in ratelimit package
Checklist
- [x] I have read the coding guide
- [ ] My change requires a documentation update and I have done it
- [x] I have added tests to cover my changes.
- [x] I have filled out the description and linked the related issues
Description
This PR fixes overlocking in Allow method which can arise when Allow is concurrently being called with two or more different keys.
Instead of locking whole functionality of Allow method we are now locking only part that changes state of limiters map, leaving individual limiter to handle synchronization individually.
This change is 
@vladopajic
So have you seen any problems with this? If this is a problem, I would like to see some pprof data to point to this. Also, we will need data on how this change improves it. For eg, if this is not a problem in the first place, we dont need the overhead of RWLock instead of normal Lock. This may not always improve things.
What overhead of RWLock do you have in mind?
The overlocking does not have anything to do with RWLock (that's additional improvement); it happens because previously this code would lock for getting/creating Limiter and AllowN method call, where later op does not need to be included in this lock.
I would also like to see performance implications of this change, but it would require a lot of time to make those. Not to mention that results could greatly differ based on node load and type of request.
Reason why I decided to make this PR was because it was trivial change that theoretically couldn't degrade performance, but in worst case scenario could have some positive impact.
What overhead of
RWLockdo you have in mind?The overlocking does not have anything to do with
RWLock(that's additional improvement); it happens because previously this code would lock for getting/creatingLimiterandAllowNmethod call, where later op does not need to be included in this lock.I would also like to see performance implications of this change, but it would require a lot of time to make those. Not to mention that results could greatly differ based on node load and type of request.
So RWLock achieves its features by consuming additional memory and synchronization primitives. Nothing comes for free. If there is no problem right now, I dont see why we should do this.
There are multiple places in the code where a lock is used this way. So, theoretically we should replace all instances of Mutex we have in the code with this, but there is no guarantee this will result in an improvement in performance.
Reason why I decided to make this PR was because it was trivial change that theoretically couldn't degrade performance, but in worst case scenario could have some positive impact.
These are dangerous assumptions when it comes to concurrency. I would consider backing these with numbers.
@aloknerurkar
Ok, I realized that utility of RWMutex is that it gives access to all concurrent reads for some cost of complexity, but I am not fully aware of those tradeoffs (I will investigate those).
Based on this input I have riverted RWMutex change leaving only overlocking part.
@mrekucci
Only intention behind this PR is to reduce unnecessary overlocking. Since this could be easily improved I decided to make this PR following boy scout rule.
With these changes I claim that code will keep same functionality while potentially improving performance (theoretically it couldn't degrade). I didn't want to go down the benchmark rabbit hole since performance was not key objective and different test sets could produce results with neglectable performance gains.
@vladopajic
Only intention behind this PR is to reduce unnecessary overlocking. Since this could be easily improved I decided to make this PR following boy scout rule.
Nobody denies
@mrekucci
Only intention behind this PR is to reduce unnecessary overlocking. Since this could be easily improved I decided to make this PR following boy scout rule.
With these changes I claim that code will keep same functionality while potentially improving performance (theoretically it couldn't degrade). I didn't want to go down the benchmark rabbit hole since performance was not key objective and different test sets could produce results with neglectable performance gains.
I apologize, you stated your intention but I'm confused about the key objective you want to achieve. When it's not performance then what is it?
@mrekucci
Cleaner and better code. As PR title would suggest key objective of this change is chore.
@mrekucci
Is there issue having this function?
This function is also part of code improvement because it makes this code
- easier to read - it requires less cognitive load to read
Allowfunction as it separates functionality for getting/creating limiter - makes future code changes less prone to overlocking
@mrekucci
The code is not so complex that it should be abstracted into a separate method/function.
This function makes it even easier to read regardless of current complexity - hence it's better code. 1*
In all cases this is desirable practice, since it makes better code (as per 1*), why would you want to suffocate it?
The second argument for a separate method/function is when it is called in multiple places, which is not the case here.
This is not true at all. There is vast number of places where function is defined even when it's being called in just one place.
It's bad practice to anticipate what will be needed in the future, if it will be needed then it might be added/refactored then.
This can lead to open ended debate.
Undeniably one needs to anticipate future (future in general; as well as code changes in this case). Even for statements 'X will not be changed in the future' one actually is anticipating future where X looks the same in future as it looks in present.
As you can see, better and more readable code is somewhat subjective to whoever is creating or reviewing it.
For some practices it requires one to adhere to it before one can see results (benefits/drawbacks).
This is simply not true. For example, there are reasons why it is desirable to have functions inline: escape-analysis (keeping all values in the stack), performance, etc... I'm not claiming that this is the case, I'm just showing that your claim is not true. In this case, as I already mentioned, I'd argue that it doesn't help with the readability, since the original code isn't complex and is compact enough.
For this case you would use inline mechanism form compiler to optimize execution. But you would still have code isolated in function.
sync.Map solution
This solution is bad because it introduces memory trashing because it always creates new limiters, rate.NewLimiter(l.rate, l.burst) is called every time. If this was not the case I would prefer that solution more.
Would it increase readability? It depends on who is reading the code and how familiar they are with sync.Map
One should get familiar with tools one needs to accomplish job. It's not shame being unfamiliar with better tools / approaches, professionals are learning and getting better everyday.
We keep going in circles here. At this point we will not be able to proceed without going through mediation.
@zelig Would you be able to be our mediator or alternatively assign one for this case?
@vladopajic
I agree with Peter that there is no need for this function. This existing function is compact enough and just moving the unlock below the if loop would be the best change.
If not that, I would say we don't need this change at all as there are no indications to say that this is a problem. Not sure if we need any mediation here.
@aloknerurkar
I can't comprehend why would you decide to block change because it encapsulated functionality in individual function.
Certainly we have deadlock situation here so mediator would be needed to help us effectively resolve and proceed.
We keep going in circles here. At this point we will not be able to proceed without going through mediation.
@zelig Would you be able to be our mediator or alternatively assign one for this case?
We are not going in circles and certainly don't need a mediator. As a reviewer, I have clearly stated what I require to be changed and why in order to give it green. You've made some arguments that I did not consider valid, and vice versa. It's okay if we disagree, but the conversation should be constructive, which I don't think it is at this point. Feel free to ask other team members for their review. Thanks for your effort and the PR 🙏
@aloknerurkar
I can't comprehend why would you decide to block change because it encapsulated functionality in individual function.
Certainly we have deadlock situation here so mediator would be needed to help us effectively resolve and proceed.
So as I mentioned in the start. The right way to do this is to first report an issue in the bee repo about some problem you are seeing. If it's related to concurrency, you should include the necessary pprof data to show the problem. Eye-balling is not the right way to go about these things. If this had been done, we would not have questioned the validity of the change.
As it is right now, I don't see this as something we should spend time on. Next time please try to create an issue first and then go about making any changes. If we think this is not a problem we would indicate it on the issue itself. That way no one's time is wasted.
I don't want to discourage you from making changes in the future. Just that we have some sort of structure to it.
@mrekucci
Thank you for your review effort.
Certainly I have closely followed all suggestions that you guys have pointed out in this process.
I would also like to draw attention to fact that I also felt that conversation slip down from constructive path even before. Original version of this comment had exclamation marks and took my response out of context forcibly trying to prove it's own agenda. Which made me to think that mediator would be needed.
I have not taken anything personally and I am still looking to work with you closely.
I have pushed another fix for overlocking without additional function definition.
Please, review again.