-
Notifications
You must be signed in to change notification settings - Fork 652
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
Compile contracts on writing them to state #4344
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we need to change the protocol here. Compiling the contracts should not affect how the protocol works (without changing the gas cost).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, see my and @bowenwang1996 comments though
nearcore/src/runtime/mod.rs
Outdated
RuntimeConfig::from_protocol_version(&self.genesis_runtime_config, protocol_version); | ||
let compiled_contract_cache: Option<Arc<dyn CompiledContractCache>> = | ||
Some(Arc::new(StoreCompiledContractCache { store: self.store.clone() })); | ||
contract_codes.par_iter().for_each(|code| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using rayon here is necessarily a good idea. At least we should limit how many cores it can use at the same time. When we state sync during catch up, the node still needs to function normally and cannot afford to spend all its cpus on this. cc @matklad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is this part running concurrently with "functioning normally" part?
As the caller of this code calls store_update.commit()
, it seems like this actually blocks normal processing, and, as such, the faster we deal with it, the faster we can resume normal operation, and, from perf pov, more threads is better.
That said, even if more threads is better for pert, we'd better to restrict the number of threads to not accidentally use more resources than we specify we need. Ie, the failure mode here is that, accidentally, near starts to require 4 cores rather than two.
I guess, my advice is:
- figure out what our actual budget here is. Can we just compile everything in one thread? If not how many threads we need at minimum?
- if there isn't strict budgeting, and we just want to go as fast as possible, we need to configure the global thread pool to use only 2 threads (or however many cores we say we need to run
neard
), to not be overly-optimistic about the actual performance. - If there is struct budgeting, and we need to hit it, we might do one of the two things:
- spawn a single thread for all background work, and submit the work to this thread. That way, the work doesn't block processing, but is still done in an efficient manner, interleaving with other operations. In this apporach, it's important that there's a thread for "background work", and not just a thread for compilation -- otherwise we might end up, in the future, with several different background workers, where each worker individually tries to not over-subscribe, but alltogether they do.
- split the work in chunks. Rather than compiling all contracts in batch, have a queue on uncompiled contracts, and, on each iteraton of the event loop, pick up up to N kilobytes worth of contract and compiling those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the caller of this code calls store_update.commit(), it seems like this actually blocks normal processing,
That is not the case. We also call this function in apply_state_part
, which is called outside of normal block processing. I don't have a specific budget in mind, but probably not a good idea to use more than half of the available cpus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I limited number of threads used here to rayon::current_num_threads() / 2
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also test the following two cases:
- A contract is deployed to some account, and after some number of epochs, a new contract is deployed to the same account. When a node state syncs after the new contract is deployed, it will have the new contract compiled locally but not the old one.
- A contract is deployed to some account and after some number of epochs, the account is deleted. Then if a node state syncs after the account is deleted, it should not have the contract compiled locally.
Put each contract to the
CompiledContractCache
when it is written to the state.This will help to decrease costs for function calls.
Plan
RuntimeAdapter::apply_state_part
for state downloaded from other nodeapply_deploy_contract
Test
Check that contract is precompiled and cached:
Runtime.apply