Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use lighter atomic counters instead of mutexes #148

Closed
wants to merge 1 commit into from

Conversation

tsenart
Copy link
Contributor

@tsenart tsenart commented Jul 2, 2014

Where appropriate, it makes sense to use lighter weight atomic counters
instead of mutexes.

@tsenart tsenart changed the title Replace mutex with atomic for rpcServer.numClients Use lighter atomic counters instead of mutexes Jul 2, 2014
@tsenart tsenart changed the title Use lighter atomic counters instead of mutexes [WIP] Use lighter atomic counters instead of mutexes Jul 2, 2014
@davecgh
Copy link
Member

davecgh commented Jul 2, 2014

We have to very careful when using atomic counters as they must be byte aligned within the struct. If you look back through history you'll find commits that specifically moved away from the atomic package for some things.

One such commit is e5a1c6e which specifically changed the byte counters you have done here as it will cause a panic.

As I stated in that commit, I really don't want to rely on having to keep track of struct alignment to prevent run-time panics.

Given the above, I'm going to vote no for this pr. It would make the code too fragile such that changing the struct definition could easily introduce run-time panics the programmer is extremely unlikely to notice.

@tsenart
Copy link
Contributor Author

tsenart commented Jul 3, 2014

Very well, assuming you want to support those architectures, I fully agree that its preferable to use a sync.Mutex for bytesReceived and bytesSent instead of keeping track of memory alignment.

For numClients, however, it is my understanding that we can safely use 32bit atomic ops.

Just for future reference, I'm including some relevant comments present in the sync/atomic/doc.go file.

// BUG(rsc): On x86-32, the 64-bit functions use instructions unavailable before the Pentium MMX.
//
// On non-Linux ARM, the 64-bit functions use instructions unavailable before the ARMv6k core.
//
// On both ARM and x86-32, it is the caller's responsibility to arrange for 64-bit
// alignment of 64-bit words accessed atomically. The first word in a global
// variable or in an allocated struct or slice can be relied upon to be
// 64-bit aligned.

Where appropriate, it makes sense to use lighter weight atomic counters
instead of mutexes.
@tsenart
Copy link
Contributor Author

tsenart commented Jul 3, 2014

I have reduced the change set accordingly.

@tsenart tsenart changed the title [WIP] Use lighter atomic counters instead of mutexes Use lighter atomic counters instead of mutexes Jul 3, 2014
@davecgh
Copy link
Member

davecgh commented Jul 3, 2014

OK

@davecgh
Copy link
Member

davecgh commented Jul 7, 2014

This has been rebased and merged as commit 2afc5a0.

@davecgh davecgh closed this Jul 7, 2014
jcvernaleo pushed a commit to jcvernaleo/btcd that referenced this pull request May 3, 2016
kcalvinalvin added a commit to kcalvinalvin/btcd that referenced this pull request Nov 29, 2024
…ing-to-utreexo-backends

blockchain, indexers: Add cache option to bridgenodes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants