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

Increase Sqlite version #775

Closed
wants to merge 13 commits into from
Closed

Conversation

MakisChristou
Copy link
Contributor

@MakisChristou MakisChristou commented Jun 18, 2024

This PR

  • Upgrades the minimum version of go from 1.21 to the latest stable version which is 1.22.4.
  • Upgrades Sqlite3 version from 3.41 to 3.45
  • Adds logdb benchmarks.

Upgrade Golang + Golangci-lint

Upgrades all golang usage to 1.22 and the linter to 1.59.0.
It's possible to upgrade golang usage to 1.22 and the linter to 1.59.1 by doing the following these commands:

go install golang.org/dl/go1.22.4@latest
go1.22.4 download
go1.22.4 env GOROOT

# Add this to your .zshrc
export GOROOT=/Users/pedro/sdk/go1.22.4/
export PATH=/Users/pedro/go/bin/:$PATH

sudo ln -sf /Users/pedro/sdk/go1.22.4/bin/go /usr/local/go/bin/go

curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.59.1
golangci-lint --version

@MakisChristou
Copy link
Contributor Author

Here are the benchmark results before after after the changes of this PR. @libotony @otherview

bench_results_formatted_docker_before.txt
bench_results_formatted_docker_after.txt

it looks like there is a slight slowdown from before the changes.

@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.58%. Comparing base (c28bd36) to head (741aa85).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #775      +/-   ##
==========================================
+ Coverage   62.50%   62.58%   +0.07%     
==========================================
  Files         199      199              
  Lines       18140    18152      +12     
==========================================
+ Hits        11338    11360      +22     
+ Misses       5719     5708      -11     
- Partials     1083     1084       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

builtin/gen/bindata.go Show resolved Hide resolved
@@ -1,6 +1,6 @@
module github.com/vechain/thor/v2

go 1.19
go 1.22.4
Copy link
Member

Choose a reason for hiding this comment

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

Should this be 1.22 instead of 1.22.4 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any reason why?

go.sum Show resolved Hide resolved
logdb/logdb_test.go Outdated Show resolved Hide resolved
logdb/logdb_test.go Outdated Show resolved Hide resolved
logdb/logdb_test.go Outdated Show resolved Hide resolved
logdb/logdb_test.go Outdated Show resolved Hide resolved
logdb/logdb_test.go Outdated Show resolved Hide resolved
Yes since its only used in that test

Co-authored-by: Pedro Gomes <[email protected]>
Co-authored-by: Pedro Gomes <[email protected]>
darrenvechain
darrenvechain previously approved these changes Jul 2, 2024
@libotony
Copy link
Member

libotony commented Jul 3, 2024

Here are the benchmark results before after after the changes of this PR. @libotony @otherview

bench_results_formatted_docker_before.txt bench_results_formatted_docker_after.txt

it looks like there is a slight slowdown from before the changes.

Summary for the benchmark result:

  • All _write ops is improving, but read ops are decreasing at the executing performance
  • All read/write ops are improving at the memory usage

logdb/logdb_bench_test.go Outdated Show resolved Hide resolved
@otherview
Copy link
Member

Here are the benchmark results before after after the changes of this PR. @libotony @otherview
bench_results_formatted_docker_before.txt bench_results_formatted_docker_after.txt
it looks like there is a slight slowdown from before the changes.

Summary for the benchmark result:

  • All _write ops is improving, but read ops are decreasing at the executing performance
  • All read/write ops are improving at the memory usage

I think the perf hit is neglatable - Also, I added https://github.com/vechain/protocol-board-repo/issues/256 where hopefully we can perhaps get some performance gains.

@libotony
Copy link
Member

libotony commented Jul 3, 2024

Here are the benchmark results before after after the changes of this PR. @libotony @otherview
bench_results_formatted_docker_before.txt bench_results_formatted_docker_after.txt
it looks like there is a slight slowdown from before the changes.

Summary for the benchmark result:

  • All _write ops is improving, but read ops are decreasing at the executing performance
  • All read/write ops are improving at the memory usage

I think the perf hit is neglatable - Also, I added vechain/protocol-board-repo#256 where hopefully we can perhaps get some performance gains.

Per my previous comment, the testing cases are not covering real use case, need to update the test case and check the result again.

@MakisChristou MakisChristou changed the title Sqlite Increase Sqlite version Jul 3, 2024
@MakisChristou MakisChristou requested a review from libotony July 4, 2024 06:15
@MakisChristou
Copy link
Contributor Author

@libotony Here are the benchmark results after incorporating the changes
bench_results_formatted_docker_after2.txt

@libotony
Copy link
Member

libotony commented Jul 4, 2024

@libotony Here are the benchmark results after incorporating the changes bench_results_formatted_docker_after2.txt

The change looks good, could you also post the same test on top of old source code?

@libotony
Copy link
Member

libotony commented Jul 4, 2024

@MakisChristou Looks like your test result is on arm based machine, I assume it's macOS, right? Could you also post the steps and the data used for testing? I would like to do a test in a Linux based system.

@MakisChristou
Copy link
Contributor Author

@libotony Here are the benchmark results after incorporating the changes bench_results_formatted_docker_after2.txt

The change looks good, could you also post the same test on top of old source code?

Yes here are the same benchmarks on the current master branch.

bench_results_formatted_docker_before2.txt

@MakisChristou
Copy link
Contributor Author

@MakisChristou Looks like your test result is on arm based machine, I assume it's macOS, right? Could you also post the steps and the data used for testing? I would like to do a test in a Linux based system.

Sure. We used the testnet_logs.db file provided by @kgapos (I think the AWS link expired so you need to reach out to him for it). The steps are the following

  1. Clone thor
  2. Choose branch (e.g. before or after the SQlite changes)
  3. Build local docker image
  4. Start docker container and map the testnet_logs.db file to the container (e.g. -v /folder/containing/testnet/logs:/home/thor/)
  5. Inside the container clone thor and run the benchmarks (e.g. go test -bench=. -benchmem -count=6 -benchtime=10s --timeout 120m github.com/vechain/thor/v2/logdb -dbPath /home/thor/testnet_logs.db)

So we are using the container that is also running thor to run the benchmarks but hopefully this is not an issue.

@libotony
Copy link
Member

libotony commented Jul 4, 2024

@MakisChristou Looks like your test result is on arm based machine, I assume it's macOS, right? Could you also post the steps and the data used for testing? I would like to do a test in a Linux based system.

Sure. We used the testnet_logs.db file provided by @kgapos (I think the AWS link expired so you need to reach out to him for it). The steps are the following

  1. Clone thor
  2. Choose branch (e.g. before or after the SQlite changes)
  3. Build local docker image
  4. Start docker container and map the testnet_logs.db file to the container (e.g. -v /folder/containing/testnet/logs:/home/thor/)
  5. Inside the container clone thor and run the benchmarks (e.g. go test -bench=. -benchmem -count=6 -benchtime=10s --timeout 120m github.com/vechain/thor/v2/logdb -dbPath /home/thor/testnet_logs.db)

So we are using the container that is also running thor to run the benchmarks but hopefully this is not an issue.

Thanks, will do a summary.

@libotony
Copy link
Member

libotony commented Jul 9, 2024

Closing in favor of #784

@libotony libotony closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants