-
Type: Technical Debt
-
Resolution: Unresolved
-
Priority: Minor - P4
-
None
-
Affects Version/s: None
-
Component/s: None
-
13
Summary
There are WT_REF_LOCK and WT_REF_UNLOCK macros, but they are not used consistently. Furthermore, there's no WT_REF_TRYLOCK, and consequently the trylock operation is open-coded in several places in different ways.
We should add a WT_REF_TRYLOCK, and we should check that all ref state changes that correspond to locking operations use the lock and unlock macros; even when this results in no functional change it makes the code easier to follow.
It is probably the case that these macros should be able to update the ref history selectively; I suspect that in some cases not using them is (or was originally) deliberate to avoid adding unnecessary refhist entries.
Furthermore, the memory barriers that lock and unlock operations should ordinarily have are not always present. It is not clear without further analysis if any of these cases are significant or not. (And this might be a can of worms and should possibly be its own ticket; that remains to be seen.)
Motivation
Tidiness and readability.
- Does this affect any team outside of WT?
No.
- How likely is it that this use case or problem will occur?
N/A, except possibly the memory barriers issue, in which case "almost certainly rarely".
- If the problem does occur, what are the consequences and how severe are they?
N/A, except possibly the memory barriers issue, in which case "almost certainly horrible and incomprehensible".
- Is this issue urgent?
No.
Acceptance Criteria (Definition of Done)
1. The tidyup part is done when all the ref state changes that correspond to lock and unlock operations but don't use the macros have been updated.
2. The concerns about refhist spam are probably best resolved by (a) making unlock operations never update the refhist (on the grounds that the ref state reflects whether the unlock has happened yet) and (b) keeping the status quo for all lock operations, subject to any consensus to the contrary in specific cases as a result of discussion or review.
3. The concerns about memory barriers need further investigation.
- Testing
Nothing out of the ordinary, I think.
- Documentation update
The architecture guide doesn't seem to mention these macros. Maybe it should though?