Prevent calling UpdateTimestampsCache if query spaces are not changed #2120
Conversation
| if (spaces.Count == 0) | ||
| return; | ||
|
|
||
| Lock.EnterUpgradeableReadLock(); |
There was a problem hiding this comment.
This is, most-likely, invalid.
There was a problem hiding this comment.
Yes, it is invalid. I will try to update the async generator to support it. I think that it is better to use EnterReadLock instead EnterUpgradeableReadLock as here doesn't seem to be needed. Also, the async version of EnterUpgradeableReadLock is harder to implement and it is also not available in the latest AsyncEx library.
There was a problem hiding this comment.
This lock upgrades to write lock a few lines below (in SetSpacesTimestampAsync).
I highly disagree with the decision to drop upgradable locks support from AsyncEx and with the reasoning behind them.
To be honest I’m biased against author of AsyncEx after they claimed that AsyncLocal does not impact performance in any how; which turned to be completely false claim in practice. So I do not accept their reasoning for dropping the upgradable locks support "developers do not need them" as valid.
I believe here is a perfect example of a need of upgradable read locks for high performance. Because we do not know the exact implementation of L2 Cache we assume the worst: it is slow. For example, L2 cache can be non local so there could be some latency in reading data. We want to allow people to read data while they are reading other data.
There was a problem hiding this comment.
My bad, I missed that. Wouldn't be better to just use (Enter\Exit)WriteLock instead of (Enter\Exit)UpgradeableReadLock inside PreInvalidate and Invalidate methods and remove the lock from the SetSpacesTimestamp as in this case it will always be upgraded, except when canceling the cancellation token when SetSpacesTimestampAsync is called which is very hard to happen.
About AyncEx, I do agree that their reasoning for droping the upgradable locks is not valid. Anyway, we will have to write our own implementation in order to avoid adding external dependencies and if we are able to avoid upgradeable locks we would have an easier job for implementing our own AsyncReadWriteLock.
Fixes #2115