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.

Sign up for GitHub

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

Jump to bottom

Refactor Benchmarking #5363

Merged
merged 35 commits into from
Feb 14, 2024
Merged

Refactor Benchmarking #5363

merged 35 commits into from
Feb 14, 2024

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Feb 8, 2024 •
edited
Loading

closes: #5099

  • Refactored the TPS loader so that it is easier to add new loads
  • Added a unit test for loads, so that we immediately know if a load is broken (this also helps with adding new loads)
  • Added an EVM transfer load type
  • This required the node bootstrap procedure to include the evm parts

@janezpodhostnik
Add EVM load
894151a 
fix tps bash scipt

add reference block to setupEVM tx
@janezpodhostnik
tps gen changes
fa4b24e
@janezpodhostnik
temp only bench last commit
6e4820b
@janezpodhostnik
fix tps server scripts
13c03a6
@janezpodhostnik
fix control script
49cc60d
@janezpodhostnik
fix transaction sender
98ebe77
@janezpodhostnik
remove compute limit
7a01db8
@janezpodhostnik
signers are not concurency safe
7979d5b
@janezpodhostnik
fix err handling
f17116e
@janezpodhostnik
fix err handling
c685c39
@janezpodhostnik
fix commit ordering
209f901
@janezpodhostnik
fix service account handling
173a00f
@janezpodhostnik
fix account creation
d8b0204
@janezpodhostnik
fix error handling
10ee110
@janezpodhostnik
fix logging
a698961
@janezpodhostnik
account creation debugging
ae74a84
@janezpodhostnik
do not reuse signers accross threads
6b65067
@janezpodhostnik
reduce create account load
1cada3d
@janezpodhostnik
use load type from parameters
a119066
@janezpodhostnik
reduce accounts created at once
323d090
@janezpodhostnik
fix evm load type name
2d185b2
@janezpodhostnik
increase timeout
3b734e1
@janezpodhostnik
add setup to evm transfer load
541905b
@janezpodhostnik
fix evm_load
7ca7296
@janezpodhostnik
make setup faster
fd10f1d
@janezpodhostnik
bugfix on account loading
433ca90
@janezpodhostnik
fix tests
d1454de
@janezpodhostnik
fix tests
7f125b6
@janezpodhostnik
reduce batch size for account creation
13f1134
@janezpodhostnik
Merge branch 'master' into janez/tps-benchmark-evm-load
f427fd2
@janezpodhostnik janezpodhostnik self-assigned this Feb 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024 •
edited
Loading

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (d22727b) 55.94% compared to head (0421a12) 55.92%.
Report is 42 commits behind head on master.

Files Patch % Lines
fvm/meter/computation_meter.go 0.00% 7 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5363      +/-   ##
==========================================
- Coverage   55.94%   55.92%   -0.02%     
==========================================
  Files        1024     1024              
  Lines       98853    98862       +9     
==========================================
- Hits        55306    55293      -13     
- Misses      39378    39399      +21     
- Partials     4169     4170       +1
Flag Coverage Δ
unittests 55.92% <22.22%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@janezpodhostnik janezpodhostnik marked this pull request as ready for review February 12, 2024 15:27
ramtinms
ramtinms reviewed Feb 12, 2024
View reviewed changes
engine/execution/state/bootstrap/bootstrap.go
Comment on lines +54 to +55
// TODO (JanezP): move this deeper
fvm.WithEVMEnabled(true),
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 still a todo for this PR or for a later time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats for a different PR, because a lot of test code needs to be touched, and this PR is large already. I'll open an issue.

ramtinms
ramtinms reviewed Feb 12, 2024
View reviewed changes
integration/benchmark/account/account.go
Comment on lines +17 to +18
PrivateKey crypto.PrivateKey
HashAlgo crypto.HashAlgorithm
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the crypto has a data structure that holds all of these values (hash, private key, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find anything. There is the AccountKey, but that has extra data.

ramtinms
ramtinms approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@ramtinms ramtinms left a comment

Choose a reason for hiding this comment

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

looks good to me just some minor comments

@janezpodhostnik janezpodhostnik mentioned this pull request Feb 13, 2024
Closed
@janezpodhostnik janezpodhostnik force-pushed the janez/tps-benchmark-evm-load branch from d8e6f95 to 4de14a3 Compare February 13, 2024 20:30
@janezpodhostnik janezpodhostnik added this pull request to the merge queue Feb 14, 2024
Merged via the queue into master with commit ceec128 Feb 14, 2024
51 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/tps-benchmark-evm-load branch February 14, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ramtinms ramtinms ramtinms approved these changes

@zhangchiqing zhangchiqing zhangchiqing approved these changes

@sideninja sideninja Awaiting requested review from sideninja

Assignees

@janezpodhostnik janezpodhostnik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Benchmark transactions running EVM operations on benchnet
4 participants
@janezpodhostnik @codecov-commenter @zhangchiqing @ramtinms