Skip to content

perf(p2p): Remove unnecessary atomic read#2950

Merged
melekes merged 1 commit intocometbft:mainfrom
osmosis-labs:dev/remove_atomic_read
May 2, 2024
Merged

perf(p2p): Remove unnecessary atomic read#2950
melekes merged 1 commit intocometbft:mainfrom
osmosis-labs:dev/remove_atomic_read

Conversation

@ValarDragon
Copy link
Contributor

@ValarDragon ValarDragon commented Apr 30, 2024 •
edited
Loading

This PR is a driveby change. We were doing an atomic read here, but this is unnecessary. Nothing in the codebase modified this variable after instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt like a minor code nit

@ValarDragon ValarDragon requested a review from a team as a code owner April 30, 2024 19:44
@ValarDragon ValarDragon requested a review from a team April 30, 2024 19:44
@ValarDragon ValarDragon changed the title Remove unnecessary atomic read perf(p2p): Remove unnecessary atomic read Apr 30, 2024
melekes
melekes approved these changes May 1, 2024
View reviewed changes
Copy link
Collaborator

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ValarDragon ❤️

@melekes melekes added backport-to-v0.37.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels May 1, 2024
@melekes melekes added this pull request to the merge queue May 2, 2024
Merged via the queue into cometbft:main with commit 9ae5886 May 2, 2024
mergify bot pushed a commit that referenced this pull request May 2, 2024
@ValarDragon @mergify
perf(p2p): Remove unnecessary atomic read (#2950)
ba9f095 
This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit

(cherry picked from commit 9ae5886)
@mergify mergify bot mentioned this pull request May 2, 2024
Merged
mergify bot pushed a commit that referenced this pull request May 2, 2024
@ValarDragon @mergify
perf(p2p): Remove unnecessary atomic read (#2950)
8a40e8e 
This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit

(cherry picked from commit 9ae5886)
mergify bot pushed a commit that referenced this pull request May 2, 2024
@ValarDragon @mergify
perf(p2p): Remove unnecessary atomic read (#2950)
9350fed 
This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit

(cherry picked from commit 9ae5886)
This was referenced May 2, 2024
Merged
Merged
melekes pushed a commit that referenced this pull request May 2, 2024
@mergify @ValarDragon
perf(p2p): Remove unnecessary atomic read (backport #2950) (#2971)
3f037ed 
This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
#2950 done by [Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
melekes pushed a commit that referenced this pull request May 2, 2024
@mergify @ValarDragon
perf(p2p): Remove unnecessary atomic read (backport #2950) (#2972)
ab9b4ef 
This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
#2950 done by [Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
melekes pushed a commit that referenced this pull request May 2, 2024
@mergify @ValarDragon
perf(p2p): Remove unnecessary atomic read (backport #2950) (#2970)
4c80a64 
This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
#2950 done by [Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
czarcas7ic pushed a commit to osmosis-labs/cometbft that referenced this pull request May 2, 2024
@mergify @ValarDragon @czarcas7ic
perf(p2p): Remove unnecessary atomic read (backport cometbft#2950) (c…
dca19ae 
…ometbft#2972)

This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
cometbft#2950 done by [Mergify](https://mergify.com).

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
@czarcas7ic czarcas7ic mentioned this pull request May 2, 2024
Merged
3 tasks
czarcas7ic added a commit to osmosis-labs/cometbft that referenced this pull request May 3, 2024
@czarcas7ic @mergify @ValarDragon
perf(p2p): Remove unnecessary atomic read (backport cometbft#2950) (c…
c1cebed 
…ometbft#2972) (#41)

This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
cometbft#2950 done by [Mergify](https://mergify.com).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
mergify bot added a commit to osmosis-labs/cometbft that referenced this pull request May 3, 2024
@czarcas7ic @mergify @ValarDragon
perf(p2p): Remove unnecessary atomic read (backport cometbft#2950) (c…
7edf825 
…ometbft#2972) (#41)

This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
cometbft#2950 done by [Mergify](https://mergify.com).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
(cherry picked from commit c1cebed)
@mergify mergify bot mentioned this pull request May 3, 2024
Merged
3 tasks
mergify bot added a commit to osmosis-labs/cometbft that referenced this pull request May 3, 2024
@czarcas7ic @mergify @ValarDragon
perf(p2p): Remove unnecessary atomic read (backport cometbft#2950) (c…
0bd15ff 
…ometbft#2972) (#41)

This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
cometbft#2950 done by [Mergify](https://mergify.com).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
(cherry picked from commit c1cebed)
@mergify mergify bot mentioned this pull request May 3, 2024
Merged
3 tasks
czarcas7ic added a commit to osmosis-labs/cometbft that referenced this pull request May 3, 2024
@mergify @czarcas7ic
perf(p2p): Remove unnecessary atomic read (backport cometbft#2950) (c…
c0c3193 
…ometbft#2972) (#41) (#50)

This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
cometbft#2950 done by [Mergify](https://mergify.com).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
(cherry picked from commit c1cebed)

Co-authored-by: Adam Tucker <adam@osmosis.team>
czarcas7ic added a commit to osmosis-labs/cometbft that referenced this pull request May 3, 2024
@mergify @czarcas7ic
perf(p2p): Remove unnecessary atomic read (backport cometbft#2950) (c…
eb525de 
…ometbft#2972) (#41) (#51)

This PR is a driveby change. We were doing an atomic read here, but this
is unnecessary. Nothing in the codebase modified this variable after
instantiation.

Doesn't really feel changelog worthy, but I can add one if we want. Felt
like a minor code nit<hr>This is an automatic backport of pull request
cometbft#2950 done by [Mergify](https://mergify.com).

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
(cherry picked from commit c1cebed)

Co-authored-by: Adam Tucker <adam@osmosis.team>
@sergio-mena
Copy link
Collaborator

sergio-mena commented May 3, 2024 •
edited
Loading

Hi, getting here a bit late...

I've checked the call hierarchy of sendSomePacketMsgs and I see that, ultimately, it's being called from two different goroutines (potentially for the same MConnection):

  • It's called from MConnection.FlushStop <-- pex.Reactor.Receive (actually, an ad-hoc goroutine there)
  • It's called from MConnection.sendRoutine, which is run as a goroutine

In principle, we could suspect there might be a race between FlushStop and sendRoutine. However, FlushStop waits on channel MConnection.doneSendRoutine before calling sendSomePacketMsgs, so we should be good.

Posting this here to further document this change.

@cason
Copy link

cason commented May 8, 2024

Just for context, calling FlushStop more than once is not a problem. The method startswith a call to stopServices. This method is protected by a lock and once the first execution of this method completes, all the following will return true. When this method returns true, the FlushStop method is a no-op.

@faddat faddat mentioned this pull request Oct 15, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@melekes melekes melekes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ValarDragon @sergio-mena @cason @melekes