Locust: Pre-generate users in FT benchmark

akashin commented 1 month ago

This PR allow to generate multiple large FT contracts that can be reused in the future benchmark runs. I ran this with 10M users per contract and was able to generate 4 contracts (one per worker), each with 10M users and 1.25GB of state in 24 hours (with gas_limit set to 10 PGas):

locust -H 127.0.0.1:3030 \
  -f locustfiles/ft.py \
  --funding-key=$KEY \
  --fixed-contract-names --num-ft-contracts=1 \
  --num-passive-users=10000000 \
  -u 4000 -r 500 --headless --processes 4

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.66%. Comparing base (48201f8) to head (91bf9ad).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #11593 +/- ## ======================================= Coverage 71.66% 71.66% ======================================= Files 788 788 Lines 161371 161371 Branches 161371 161371 ======================================= + Hits 115644 115649 +5 + Misses 40693 40686 -7 - Partials 5034 5036 +2 ``` | [Flag](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | Coverage Δ | | |---|---|---| | [backward-compatibility](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <ø> (ø)` | | | [db-migration](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <ø> (ø)` | | | [genesis-check](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.36% <ø> (ø)` | | | [integration-tests](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `37.84% <ø> (-0.06%)` | :arrow_down: | | [linux](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `69.04% <ø> (-0.03%)` | :arrow_down: | | [linux-nightly](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.15% <ø> (-0.01%)` | :arrow_down: | | [macos](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `52.65% <ø> (+1.57%)` | :arrow_up: | | [pytests](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.59% <ø> (ø)` | | | [sanity-checks](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.39% <ø> (ø)` | | | [unittests](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `66.25% <ø> (+<0.01%)` | :arrow_up: | | [upgradability](https://app.codecov.io/gh/near/nearcore/pull/11593/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.28% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

akashin commented 4 weeks ago

What do we need this for? In particular, is the pre-generation of state from #11560 not enough, considering we said we don’t want to generate users upon each benchmark run?

We need to get the created users involved in each benchmark run, otherwise it's just an idle state that is never touched and is not occupying space in caches that we are trying to test by increasing the state size.

Is there any recipe on how to reuse the state created by https://github.com/near/nearcore/pull/11560 in a FT benchmark run?

Ekleog-NEAR commented 4 weeks ago

otherwise it's just an idle state that is never touched and is not occupying space in caches that we are trying to test by increasing the state size

Hmm so it’s still making the trie bigger even if it’s never touched, and by making the trie bigger it increases the touching-trie-node costs. This being said, you’re right that it could be made worse, by actually using all the passive users of the state, instead of just the few passive users from the benchmark.

In order to properly reuse all the pre-created state, we should run this line for each account that has been generated in the state, before starting the benchmark: https://github.com/near/nearcore/blob/49eff7083e9f02b2fb63e063ed332b8188bf7d89/pytest/tests/loadtest/locust/common/ft.py#L58

This comes from register_passive_users, and AFAIU should be enough for the benchmark to try using all the pre-generated passive users immediately, instead of trying to recreate them on each run.

Does that make sense?

akashin commented 4 weeks ago

otherwise it's just an idle state that is never touched and is not occupying space in caches that we are trying to test by increasing the state size

Hmm so it’s still making the trie bigger even if it’s never touched, and by making the trie bigger it increases the touching-trie-node costs. This being said, you’re right that it could be made worse, by actually using all the passive users of the state, instead of just the few passive users from the benchmark.

In order to properly reuse all the pre-created state, we should run this line for each account that has been generated in the state, before starting the benchmark:

https://github.com/near/nearcore/blob/49eff7083e9f02b2fb63e063ed332b8188bf7d89/pytest/tests/loadtest/locust/common/ft.py#L58

This comes from register_passive_users, and AFAIU should be enough for the benchmark to try using all the pre-generated passive users immediately, instead of trying to recreate them on each run.

Does that make sense?

It would be interesting to see the difference between running with idle large state and the hot large state. But ultimately, the benchmark needs to use hot large state to reflect disk access costs.

High-level idea makes sense, but I'm not sure how to implement it for https://github.com/near/nearcore/pull/11560, hence I created this PR.

Ekleog-NEAR commented 4 weeks ago

It would be interesting to see the difference between running with idle large state and the hot large state.

This should be easy: the way I see it, we would add a --pregenerated-users=N that’d assume the users numbered 0..N are pre-created (with the account name being hash(i)). We could then run with lots of pregenerated users anyway, but run the benchmark with either --pregenerated-users=500 or --pregenerated-users=10000000 :)

High-level idea makes sense, but I'm not sure how to implement it for https://github.com/near/nearcore/pull/11560, hence I created this PR.

Do you want to have a 1:1 so I can explain the details of the idea? :) That said if within the next 1-2 weeks you don’t think you’ll have time to look into it, then I’ll just do it before re-generating the state. The rough steps are:

Make sure that all accounts are generated as hash(i) with i in 0..N
Add the --pregenerated-users=M option to the actual benchmark, that just appends hash(i) for i in 0..M into registered_users
Probably we’ll need to add an option to #11560 to stop generating accounts after some number is reached, or even generate exactly users X..Y. This will make us able to be sure that all the pre-generated users are actually pre-generated, as right now it generates as much state as possible but without checking that all FT contracts are actually balanced

akashin commented 3 weeks ago

@Ekleog-NEAR , I tweaked parameters a bit more for this to work well on GCP and it should be ready for submission.

akashin commented 3 weeks ago

The changes to create_passive_users sound reasonable to me.

However, there’s one thing bothering me: right now you add a --num-passive-users option that generates passive users at the beginning. In order to tie in properly with #11560, WDYT about this?
* Renaming this to `--pre-generate-passive-users=N`

* In follow-up work, adding a `--pre-existing-passive-users=N` option that’d just do the second point from [this comment](https://github.com/near/nearcore/pull/11593#issuecomment-2180692999)
Overall I’m not even sure --pre-generate-passive-users would be needed, considering #11560 and the fact that pre-generating is too slow anyway; but at least with that name it wouldn’t conflict with the future --pre-existing-passive-users :)

At the moment you can re-use users just by passing num-passive-users - it's not ideal as it is doing some redundant work, but it is still much faster to run on a database where users are already created (it mostly queries accounts for existence and doesn't try to create them). I plan to optimize this a bit further in the future to leverage the fact that accounts are created sequentially and we can do a binary search to find how much more accounts we need to create.

Ekleog-NEAR commented 3 weeks ago

it mostly queries accounts for existence and doesn't try to create them

If this is actually fast enough, then it would be great! But I’m not sure how you reached that conclusion? From my reading of the code, what should happen is for each num-passive-user requested for, regardless of how many users already exist, on locust init:

it calls prepare_accounts, which unconditionally calls sign_create_account_with_full_access_key_and_balance_transaction and submits the resulting transaction
it calls register_passive_user, which unconditionally submits a function call transaction to deposit some storage

And both of these operations should take both wall clock time and gas, thus making creating them all very slow.

OTOH, if the users already exist, it should be enough to just run self.registered_users.append(account.key.account_id) as per my above comment, which is basically instantaneous. And we can offload figuring that out to the user, because in practice we’ll know how many users are pregenerated in our databases, hence my suggestion for the parameter renaming right now, and filing an issue to add the --pre-existing-passive-users option.

So I’m not sure, where did I miss the check for pre-existence of accounts to not re-create them?

akashin commented 3 weeks ago

it mostly queries accounts for existence and doesn't try to create them

If this is actually fast enough, then it would be great! But I’m not sure how you reached that conclusion? From my reading of the code, what should happen is for each num-passive-user requested for, regardless of how many users already exist, on locust init:
1. it calls `prepare_accounts`, which unconditionally calls `sign_create_account_with_full_access_key_and_balance_transaction` and submits the resulting transaction

2. it calls `register_passive_user`, which unconditionally submits a function call transaction to deposit some storage
And both of these operations should take both wall clock time and gas, thus making creating them all very slow.

OTOH, if the users already exist, it should be enough to just run self.registered_users.append(account.key.account_id) as per my above comment, which is basically instantaneous. And we can offload figuring that out to the user, because in practice we’ll know how many users are pregenerated in our databases, hence my suggestion for the parameter renaming right now, and filing an issue to add the --pre-existing-passive-users option.

So I’m not sure, where did I miss the check for pre-existence of accounts to not re-create them?

sign_create_account_with_full_access_key_and_balance_transaction is no unconditional, it is only called for accounts that need to be created https://github.com/near/nearcore/blob/ea9765f9f91ad4d38cdd4cdfa224b2037c7a19c5/pytest/tests/loadtest/locust/common/base.py#L504 (see iteration over to_create list). Existing accounts will not be there and instead will be in to_refresh list https://github.com/near/nearcore/blob/ea9765f9f91ad4d38cdd4cdfa224b2037c7a19c5/pytest/tests/loadtest/locust/common/base.py#L491 for which we only refresh nonces which does not submit any new transactions https://github.com/near/nearcore/blob/ea9765f9f91ad4d38cdd4cdfa224b2037c7a19c5/pytest/tests/loadtest/locust/common/base.py#L531

I agree that point 2 is not perfect and we should be able to avoid it, but these transactions should be no-op as the account is already created https://github.com/near/near-sdk-rs/blob/c843e9aec747b9acc7e240a6dea80a14448a7bfd/near-contract-standards/src/fungible_token/storage_impl.rs#L56

We can offload this to the user, but it will be more error-prone as it creates a possibility of using an incorrect value and getting hard-to-decipher failures.

Ekleog-NEAR commented 3 weeks ago

Got it, thank you for the precision on step 1! I had indeed missed the split into to_create and to_refresh, so that only leaves one function call transaction per already-created user.

Dichotomy would likely be the best option, but this may be good enough for now, you know better than I as you actually tried running it. If it is, maybe we should delete the outcome of #11560 altogether? There’s no point in keeping the two of these PRs.

akashin commented 3 weeks ago

Got it, thank you for the precision on step 1! I had indeed missed the split into to_create and to_refresh, so that only leaves one function call transaction per already-created user.

Dichotomy would likely be the best option, but this may be good enough for now, you know better than I as you actually tried running it. If it is, maybe we should delete the outcome of #11560 altogether? There’s no point in keeping the two of these PRs.

We shouldn't need ft-state-builder.py any more, I agree.

near / nearcore

Locust: Pre-generate users in FT benchmark #11593

Codecov Report