Add thread safety to stats #719
Conversation
twcrone
force-pushed
the
tcrone/add-thread-safety-to-stats
branch
from
b219d22
to
ef15db0
Compare
| return count > 0 || total > 0 || totalExclusive > 0; | ||
| boolean hasData; | ||
| synchronized (lock) { | ||
| hasData = count > 0 || total > 0 || totalExclusive > 0; |
There was a problem hiding this comment.
possibly fewer branches: (count | total | totalExclusive) != 0. Probably violates your code style, I'd expect.
this is also cacheable. in all your mutators you could flip a stored hasData field and since booleans are atomic, you could mark volatile and avoid a lock. Micro-optimization and I'd only consider if this were in critical piece of code
There was a problem hiding this comment.
cool...yeah this is existing logic so I was avoiding changing it but might be worth it...I was just adding synchronize around things that were being changed
| } | ||
|
|
||
| @Override | ||
| public float getTotal() { | ||
| return total; | ||
| synchronized (lock) { | ||
| return total; |
There was a problem hiding this comment.
reading a float is atomic. if you mark volatile, you could avoid the locking here.
There was a problem hiding this comment.
ok, cool...played with volatile but wasn't sure.
There was a problem hiding this comment.
So overall, the synchronized + lock object is correct & minimal. Adheres to "Principle of Least Surprise" and easiest to read and adheres to "Easy to Change" from PragProg 20th AE.
There are some possible micro-optimizations leveraging volatile and primitives the JVM guarantees to be atomic. However these are typically extra cognitive load and only warranted if the code's really performance critical.
Unrelated to concurrency: This class doesn't seem to handle overflow conditions.
Yeah, we actually are seeing that some as well I think but thought it was in part because of all the updates to state that wasn't thread-safe. Where is a good design/pattern for handling overflow conditions? |
twcrone
force-pushed
the
tcrone/add-thread-safety-to-stats
branch
from
0d4688b
to
e85c7ce
Compare
|
This did not fix the issue and we think thread safety is handled at a higher level. |
Overview
Fix thread safety issues in stats collections
Related Github Issue
#709
Testing
TBD
Checks
[ ] Are your contributions backwards compatible with relevant frameworks and APIs?
[ ] Does your code contain any breaking changes? Please describe.
[ ] Does your code introduce any new dependencies? Please describe.