[repo] Small cleanup/simplification #6048
Conversation
github-actions
bot
added
pkg:OpenTelemetry.Api.ProviderBuilderExtensions
| { | ||
| var sw = default(SpinWait); | ||
| while (true) | ||
| { | ||
| sw.SpinOnce(); | ||
|
|
||
| double currentValue = Volatile.Read(ref location); | ||
| var returnedValue = Interlocked.CompareExchange(ref location, currentValue + value, currentValue); |
There was a problem hiding this comment.
Doesn't just refetching the new value break the compare exchange pattern?
If that is not needed, you can just overwrite what is there, without looking at the current value.
There was a problem hiding this comment.
Not sure if I understood the question correctly, but the new value is calculated based on the current value. Previously it used currentValue from the previous failed compare-exchange, but after spin waiting, which doesn't make much sense since the value is quite old by that point (and thus likely to fail again).
There was a problem hiding this comment.
I haven't seen spin waits used with compare exchange in simple flows like this where it's guaranteed that forward progress is made even under heavy contention. So maybe should remove the spin wait and then it would make sense to use the compare exchange result for current value.
There was a problem hiding this comment.
I removed the spin wait since just doing compare exchange in a loop is more efficient and produces smaller machine code.
| @@ -36,14 +36,14 @@ public DirectorySizeTracker(long maxSizeInBytes, string path) | |||
| /// <returns>True if space is available else false.</returns> | |||
| public bool IsSpaceAvailable(out long currentSizeInBytes) | |||
| { | |||
| currentSizeInBytes = Interlocked.Read(ref this.directoryCurrentSizeInBytes); | |||
| currentSizeInBytes = Volatile.Read(ref this.directoryCurrentSizeInBytes); | |||
There was a problem hiding this comment.
Why?
Is this still safe on 32 bit machines?
There was a problem hiding this comment.
From the docs: "Volatile reads and writes on such 64-bit memory are atomic even on 32-bit processors, unlike regular reads and writes."
It's significantly faster to use volatile reads (that are basically as fast as regular reads with just some additional guarantees about memory ordering) than interlocked operations that take exclusive ownership of the cache line and take at least 40 CPU cycles.
|
@pentp There's some stuff on here I would be happy to merge. Like adding |
CodeBlanch
changed the title
Some basic cleanup and simplification.