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

Bring current with SDK internalization #6

Closed

Conversation

faddat
Copy link
Member

@faddat faddat commented Jan 14, 2022

Marko began the re-organization mentioned in #2 and this PR merges changes from his branch in cosmos-sdk/iavl with those found here.

These are mainly ease of use changes that make the iavl library more readable, BUT also adds ics23 support and a few build files and bumps tendermint itself to 0.35.0

@ValarDragon for Osmosis v7, we're planning to use 0.34.x + 0.45.0, correct?

yun-yeo and others added 10 commits December 1, 2021 10:55
* add changelog entry for 0.17.3

* Update CHANGELOG.md
…ffer (cosmos#452)

After a bytes.Buffer has been freshly created, there is no need to
invoke .Reset.

Fixes cosmos#451
* reuse buffer

* copy bytes
…mos#456)

The prior code used a naive string concatenation
str += "..."

which is very slow and inefficient especially when iterating over
all the keys, but also it performed an unnecessary byteslice->string
conversion for all arguments inside (nodeDB).traverse* using

    str += fmt.Sprintf("%s: %x\n", string(key), value)

notice the `string(key)` passed into fmt.Sprintf("%s"?
At bare minimum that code should just simply be:

    str += fmt.Sprintf("%s: %x\n", key, value)

per https://twitter.com/orijtech/status/1462100381803102213?s=20
which saves a whole lot of cycles and RAM.

This change uses:
* (*bytes.Buffer) in combination with fmt.Fprintf
* A sync.Pool with (*bytes.Buffer) values to reuse buffers

and the results are profound:

```shell
$ benchstat before.txt after.txt
name          old time/op    new time/op    delta
TreeString-8     111ms ± 8%       4ms ± 4%  -96.01%  (p=0.000 n=20+20)

name          old alloc/op   new alloc/op   delta
TreeString-8     734MB ± 0%       2MB ± 1%  -99.72%  (p=0.000 n=20+20)

name          old allocs/op  new allocs/op  delta
TreeString-8     37.0k ± 0%     28.7k ± 0%  -22.40%  (p=0.000 n=20+20)
```

Fixes cosmos#455
Bumps [docker/login-action](https://github.com/docker/login-action) from 1.10.0 to 1.12.0.
- [Release notes](https://github.com/docker/login-action/releases)
- [Commits](docker/login-action@v1.10.0...v1.12.0)

---
updated-dependencies:
- dependency-name: docker/login-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@faddat faddat marked this pull request as draft January 14, 2022 15:15
Copy link
Member Author

@faddat faddat left a comment

Choose a reason for hiding this comment

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

In addition to the changes mentioned so far this adds some build tooling, updates the changelog, and adds ics23 support.

@tac0turtle
Copy link

can we keep it in the upstream then just update it once merged. Ideally we close this fork when the work is done. I believe this fork was meant for permissions right @ValarDragon ?

@faddat
Copy link
Member Author

faddat commented Jan 20, 2022

absolutely :).

@faddat faddat closed this Jan 20, 2022
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.

5 participants