This repository has been archived by the owner on Jul 31, 2023. It is now read-only.
lockless version of defaultIDGenerator.NewSpanID #851
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
semistrict
approved these changes
Aug 1, 2018
trace/trace.go
Outdated
| @@ -479,13 +479,10 @@ type defaultIDGenerator struct { | |||
| // NewSpanID returns a non-zero span ID from a randomly-chosen sequence. | |||
| // mu should be held while this function is called. | |||
There was a problem hiding this comment.
Is this statement still true?
There was a problem hiding this comment.
Thanks for noticing, removing it.
odeke-em
approved these changes
Aug 2, 2018
odeke-em
added a commit
to orijtech/opencensus-go
that referenced
this pull request
Aug 16, 2018
Fixes census-instrumentation#587 This bug manifested as a consequence of https://golang.org/pkg/sync/atomic/#pkg-note-BUG and was exposed by PR census-instrumentation#851 which switched to atomically incrementing defaultIDGenerator.nextSpanID The organization of the fields was misaligned on 32-bit machines because the field `traceIDRand *rand.Rand`, a pointer was included as the second field of the struct. This is because the size of a pointer on 32-bit machines is 4 bytes, hence after the second field, we'll have offset from 12 bytes and for atomic access of *int64 fields, which are accessed in 4 byte increments by atomic operations, on 32-bit machines, their addresses are on non-8-byte divisible alignments i.e. nextSpanID -- [28, 36] spanIDInc -- [36, 44] but on 64-bit machines, sizeof(pointer) = 8 bytes hence their addresses are on 8-byte divisible alignments i.e. nextSpanID -- [32, 40] spanIDInc -- [40, 48] Thus the required reorganization but making the pointer the last field fixes the problem for both 32-bit and 64-bit. This fix can be verified by prefixing `GOARCH=386` before running code or tests.
odeke-em
added a commit
to orijtech/opencensus-go
that referenced
this pull request
Aug 16, 2018
Fixes census-instrumentation#865 This bug manifested as a consequence of https://golang.org/pkg/sync/atomic/#pkg-note-BUG and was exposed by PR census-instrumentation#851 which switched to atomically incrementing defaultIDGenerator.nextSpanID The organization of the fields was misaligned on 32-bit machines because the field `traceIDRand *rand.Rand`, a pointer was included as the second field of the struct. This is because the size of a pointer on 32-bit machines is 4 bytes, hence after the second field, we'll have offset from 12 bytes and for atomic access of *int64 fields, which are accessed in 4 byte increments by atomic operations, on 32-bit machines, their addresses are on non-8-byte divisible alignments i.e. nextSpanID -- [28, 36] spanIDInc -- [36, 44] but on 64-bit machines, sizeof(pointer) = 8 bytes hence their addresses are on 8-byte divisible alignments i.e. nextSpanID -- [32, 40] spanIDInc -- [40, 48] Thus the required reorganization but making the pointer the last field fixes the problem for both 32-bit and 64-bit. This fix can be verified by prefixing `GOARCH=386` before running code or tests.
odeke-em
added a commit
to orijtech/opencensus-go
that referenced
this pull request
Aug 16, 2018
Fixes census-instrumentation#865 This bug manifested as a consequence of https://golang.org/pkg/sync/atomic/#pkg-note-BUG and was exposed by PR census-instrumentation#851 which switched to atomically incrementing defaultIDGenerator.nextSpanID The organization of the fields was misaligned on 32-bit machines because the field `traceIDRand *rand.Rand`, a pointer was included as the second field of the struct. This is because the size of a pointer on 32-bit machines is 4 bytes, hence after the second field, we'll have offset from 12 bytes and for atomic access of *int64 fields, which are accessed in 4 byte increments by atomic operations, on 32-bit machines, their addresses are on non-8-byte divisible alignments i.e. * nextSpanID -- [28, 36] * spanIDInc -- [36, 44] but on 64-bit machines, sizeof(pointer) = 8 bytes hence their addresses are on 8-byte divisible alignments i.e. * nextSpanID -- [32, 40] * spanIDInc -- [40, 48] Thus the required reorganization but making the pointer the last field fixes the problem for both 32-bit and 64-bit. This fix can be verified by prefixing `GOARCH=386` before running code or tests.
semistrict
pushed a commit
that referenced
this pull request
Aug 16, 2018
Fixes #865 This bug manifested as a consequence of https://golang.org/pkg/sync/atomic/#pkg-note-BUG and was exposed by PR #851 which switched to atomically incrementing defaultIDGenerator.nextSpanID The organization of the fields was misaligned on 32-bit machines because the field `traceIDRand *rand.Rand`, a pointer was included as the second field of the struct. This is because the size of a pointer on 32-bit machines is 4 bytes, hence after the second field, we'll have offset from 12 bytes and for atomic access of *int64 fields, which are accessed in 4 byte increments by atomic operations, on 32-bit machines, their addresses are on non-8-byte divisible alignments i.e. * nextSpanID -- [28, 36] * spanIDInc -- [36, 44] but on 64-bit machines, sizeof(pointer) = 8 bytes hence their addresses are on 8-byte divisible alignments i.e. * nextSpanID -- [32, 40] * spanIDInc -- [40, 48] Thus the required reorganization but making the pointer the last field fixes the problem for both 32-bit and 64-bit. This fix can be verified by prefixing `GOARCH=386` before running code or tests.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without resorting to common patterns of reusing lower bits of TraceID for a root SpanID as I proposed in #850 we can actually get a lock free version of the nextSpanID logic as currently implemented for all calls to NewSpanID. This helps with #654.