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

use the mock_node framework in a benchmark #6778

Merged
merged 9 commits into from
May 11, 2022

Conversation

marcelo-gonzalez
Copy link
Contributor

@marcelo-gonzalez marcelo-gonzalez commented May 10, 2022

This sets up sync performance benchmarks using the existing mock_node tool. The basic setup right now is that we check in tarballs containing sample home dirs in the mock_node/benches directory, and then start a node that will sync from a mock network feeding blocks from that home directory, measuring the time it takes for view_client.send(GetBlock::latest()) to pass a certain height.

Checking in tarballs like this will only be practical for small sample directories, but it should be possible to use the code here to run benchmarks on larger sample directories not checked in to git, possibly with the help of some other tool.

run with cargo bench -p mock_node -- mock_node_sync_empty to run the quicker benchmark, or omit the testname to run all.

@marcelo-gonzalez marcelo-gonzalez requested a review from a team as a code owner May 10, 2022 03:50
@marcelo-gonzalez marcelo-gonzalez marked this pull request as draft May 10, 2022 03:50
@marcelo-gonzalez marcelo-gonzalez marked this pull request as ready for review May 10, 2022 20:13
@marcelo-gonzalez
Copy link
Contributor Author

@mzhangmzz @mm-near ok, removed the draft status. PTAL!

tools/mock_node/benches/sync.rs Show resolved Hide resolved
tools/mock_node/benches/sync.rs Show resolved Hide resolved
tools/mock_node/benches/sync.rs Outdated Show resolved Hide resolved
tools/mock_node/benches/sync.rs Outdated Show resolved Hide resolved
tools/mock_node/benches/sync.rs Outdated Show resolved Hide resolved
tools/mock_node/benches/sync.rs Outdated Show resolved Hide resolved
tools/mock_node/src/setup.rs Show resolved Hide resolved
tools/mock_node/benches/sync.rs Show resolved Hide resolved
@near-bulldozer near-bulldozer bot merged commit dddbb6a into near:master May 11, 2022
@marcelo-gonzalez marcelo-gonzalez deleted the mock-bench branch May 26, 2022 21:42
@matklad
Copy link
Contributor

matklad commented May 27, 2022

Hm, this PR added 25mb binary blob to the shared git repository. This is less than ideal, as git is pretty bad at dealing with large files, in a sense that now everyone needs to download extra 25 mbs when cloning the repo. This seems like a rather large cost, considering we have to pay it indefinitelly (even if we remove the file, it'll still be in the history).

I suggest begin more careful with large files in the future. Perhaps we should add a CI check that there isn't anything lanrger than 0.5 meg in the repo?

@marcelo-gonzalez
Copy link
Contributor Author

Hm, this PR added 25mb binary blob to the shared git repository. This is less than ideal, as git is pretty bad at dealing with large files, in a sense that now everyone needs to download extra 25 mbs when cloning the repo. This seems like a rather large cost, considering we have to pay it indefinitelly (even if we remove the file, it'll still be in the history).

I suggest begin more careful with large files in the future. Perhaps we should add a CI check that there isn't anything lanrger than 0.5 meg in the repo?

Ah true :(. Yeah I think a check like that is a good idea

marcelo-gonzalez added a commit to marcelo-gonzalez/nearcore that referenced this pull request Mar 25, 2023
This was added in near#6778, but
it's honestly really not needed. There's been some discussion that
it's easier to just run mock-node directly, and that the benchmarks
don't add a ton of value. Also it's not great to have added binary
files to the tree.
near-bulldozer bot pushed a commit that referenced this pull request Mar 27, 2023
This was added in #6778, but it's honestly really not needed. There's been some discussion that it's easier to just run mock-node directly, and that the benchmarks don't add a ton of value. Also it's not great to have added binary files to the tree.
nikurt pushed a commit that referenced this pull request Mar 27, 2023
This was added in #6778, but it's honestly really not needed. There's been some discussion that it's easier to just run mock-node directly, and that the benchmarks don't add a ton of value. Also it's not great to have added binary files to the tree.
nikurt pushed a commit that referenced this pull request Mar 29, 2023
This was added in #6778, but it's honestly really not needed. There's been some discussion that it's easier to just run mock-node directly, and that the benchmarks don't add a ton of value. Also it's not great to have added binary files to the tree.
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Apr 5, 2023
This was added in near#6778, but it's honestly really not needed. There's been some discussion that it's easier to just run mock-node directly, and that the benchmarks don't add a ton of value. Also it's not great to have added binary files to the tree.
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.

3 participants