Skip to content

do not pin deps to exact versions#2844

Merged
ebuchman merged 1 commit intodevelopfrom
anton/deps
Nov 15, 2018
Merged

do not pin deps to exact versions#2844
ebuchman merged 1 commit intodevelopfrom
anton/deps

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Nov 15, 2018

(at least those with releases)

because

  • they are locked in .lock file already
  • individual dependencies can be updated with dep ensure -update XXX
  • review process (and ^^^) should help us prevent accidental updates

Closes #2798

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

because
- they are locked in .lock file already
- individual dependencies can be updated with `dep ensure -update XXX`
- review process (and ^^^) should help us prevent accidental updates

Closes #2798
@codecov-io
Copy link

Codecov Report

Merging #2844 into develop will increase coverage by 0.18%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2844      +/-   ##
===========================================
+ Coverage    62.23%   62.41%   +0.18%     
===========================================
  Files          212      212              
  Lines        17255    17224      -31     
===========================================
+ Hits         10738    10750      +12     
+ Misses        5614     5572      -42     
+ Partials       903      902       -1
Impacted Files Coverage Δ
privval/tcp_server.go 78.57% <0%> (-2.86%) ⬇️
libs/db/remotedb/grpcdb/server.go 0% <0%> (ø) ⬆️
consensus/reactor.go 67.89% <0%> (+0.23%) ⬆️
consensus/state.go 80.02% <0%> (+0.23%) ⬆️
consensus/wal_generator.go 79.43% <0%> (+0.27%) ⬆️
p2p/pex/pex_reactor.go 72.99% <0%> (+0.32%) ⬆️

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

This is great! I think this is a good idea, since why would we enforce the exact revision of these libs for any tm dependency.

[[constraint]]
name = "google.golang.org/grpc"
version = "=1.13.0"
version = "~1.13.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being considered a serialization library? We should move it to the other section then.

@ebuchman ebuchman merged commit e93b492 into develop Nov 15, 2018
@ebuchman ebuchman deleted the anton/deps branch November 15, 2018 19:40
iKapitonau pushed a commit to scrtlabs/tendermint that referenced this pull request Jul 16, 2024
…sensus (backport tendermint#3003) (tendermint#3082)

Closes tendermint#2844

We are seeing that the blockstore loading operations get used in hot
loops within gossip routines, and queryMaj23 routines. This PR reduces
that overhead using an LRU cache.

The LRU cache does have a mutex on every get, but the time with the LRU
cache is 9x faster than without (before even adding in DB overheads),
due to the proto unmarshalling saved. We could imagine a setup where we
avoided a lock there entirely. I don't think this is worth right now,
since the new code is 9x faster, and these mostly appear in catchup code
which should not be highly contended for across peers at the same time.

With the new benchmark I added:
OLD:
```
BenchmarkRepeatedLoadSeenCommit-12         24447             54691 ns/op           46495 B/op        319 allocs/op
```
NEW:
```
BenchmarkRepeatedLoadSeenCommit-12        224131              6401 ns/op            8320 B/op          2 allocs/op
```

It turns out these gossip routines don't need mutative copies, so we
could optimize out the large allocation in the future if we want.

1 hour cpu profile that shows this appearing in prod: 

![image](https://github.com/cometbft/cometbft/assets/6440154/5a7e0f02-8385-4c01-aa6a-dba2a2bf376d)

The state machine execution time here for context is 92 seconds. So this
is adding up in system load (and GC! The GC load is 52GB, the entire
trace is 200GB, with other parts being optimized down from recent PRs)

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request tendermint#3003 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
cboh4 pushed a commit to scrtlabs/tendermint that referenced this pull request Apr 7, 2025
…sensus (backport tendermint#3003) (tendermint#3083)

Closes tendermint#2844

We are seeing that the blockstore loading operations get used in hot
loops within gossip routines, and queryMaj23 routines. This PR reduces
that overhead using an LRU cache.

The LRU cache does have a mutex on every get, but the time with the LRU
cache is 9x faster than without (before even adding in DB overheads),
due to the proto unmarshalling saved. We could imagine a setup where we
avoided a lock there entirely. I don't think this is worth right now,
since the new code is 9x faster, and these mostly appear in catchup code
which should not be highly contended for across peers at the same time.

With the new benchmark I added:
OLD:
```
BenchmarkRepeatedLoadSeenCommit-12         24447             54691 ns/op           46495 B/op        319 allocs/op
```
NEW:
```
BenchmarkRepeatedLoadSeenCommit-12        224131              6401 ns/op            8320 B/op          2 allocs/op
```

It turns out these gossip routines don't need mutative copies, so we
could optimize out the large allocation in the future if we want.

1 hour cpu profile that shows this appearing in prod: 

![image](https://github.com/cometbft/cometbft/assets/6440154/5a7e0f02-8385-4c01-aa6a-dba2a2bf376d)

The state machine execution time here for context is 92 seconds. So this
is adding up in system load (and GC! The GC load is 52GB, the entire
trace is 200GB, with other parts being optimized down from recent PRs)

---

#### PR checklist

- [ ] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [ ] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request tendermint#3003 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
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.

4 participants