runtime: implement a receipt preparation pipeline #11839
Conversation
nagisa
force-pushed
the
contract-preparation-pipeline
branch
2 times, most recently
from
fa024d1
to
0c1af4a
Compare
As a somewhat notable shift in the approach, we'll start focusing on contract wasm code management by directly addressing it by the code hash, rather than going through The State (Trie). One notable snag here is that the transaction runtime doesn't actually have a write access to the underlying storage, so we continue relying on the `TrieUpdate` to write these contracts out. In the meanwhile we maintain an in-memory map of deployed contracts to make the storage appear consistent.
nagisa
force-pushed
the
contract-preparation-pipeline
branch
from
0c1af4a
to
6c57adf
Compare
I'm seeing a lot of integration tests failing, not sure if its the setup or what... In particular suspect is the `yield_then_resume` test which fails to link `sleep_nanos` but then it is also me running with integration tests, so why is that host function coming up at all...? EDIT: so `nextest-integration nightly` passes, but not `stable`. Which suggests that somewhere passing of the config or whatnot is not working out quite right…
I suspect that it is not actually exercised by our integration tests however... time to write some, but lets give coverage a spin.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11839 +/- ##
==========================================
- Coverage 71.80% 71.65% -0.15%
==========================================
Files 796 809 +13
Lines 163372 164549 +1177
Branches 163372 164549 +1177
==========================================
+ Hits 117312 117914 +602
- Misses 41006 41569 +563
- Partials 5054 5066 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
nagisa
requested review from
saketh-are,
pugachAG and
Ekleog-NEAR
and removed request for
saketh-are
|
r? @Ekleog-NEAR for the runtimeish bits This one I think is better reviewed as a whole PR (files are nicely separated and commits -- less so.) Feel free to note if you think any of the FIXMEs or TODOs are critical to resolve before this can land. Side note: I'm surprised by how good our existent coverage is. Most of the uncovered lines are actually just panics :D |
| entry.insert(Arc::clone(&task)); | ||
| // FIXME: don't spawn all tasks at once. We want to keep some capacity for | ||
| // other things and also to control (in a way) the concurrency here. | ||
| rayon::spawn_fifo(move || { |
There was a problem hiding this comment.
Should there be a specific rayon thread pool for the pipeline manager, to avoid racing tasks with other task pushers, and instead use the os preemptive scheduling?
There was a problem hiding this comment.
I don't think we want to have a separate thread pool, no. We already have an excess of thread pools with NCPU threads in each. But even if the pool is occupied with other business, the implementation here won't wait on it unnecessarily.
There was a problem hiding this comment.
My thinking was actually the opposite: it seems to me that this pipelining prefetch business could be busying the thread pool for other, more pressing business. And by limiting the pipelining prefetch thread pool to 1 thread, we could also make sure it doesn't take up too much compute.
This being said, feel free to ignore the suggestion, it's most likely not going to be anything critical :)
There was a problem hiding this comment.
Fair enough, I realize now that this is actually more aggressive in scheduling new work than I meant it to be, so I'll need to rewrite this anyhow. Limiting the pool, however, isn't quite the right approach IMO.
runtime/runtime/src/lib.rs
Outdated
|
|
||
| let mut local_receipt_iter = local_receipts.iter(); | ||
| let mut prep_lookahead_iter = local_receipts.iter(); | ||
| let _ = prep_lookahead_iter.next(); // stagger preparation to not prepare the first receipt. |
There was a problem hiding this comment.
what if the first receipt contains deploy contract action, should we block the account_id for further pipelining in that case?
There was a problem hiding this comment.
Good eye! I think I addressed this by not trying to be as smart. I should add a regression test for this too (please do not resolve this thread quite yet.)
@Ekleog-NEAR I also made changes which I think will make it clearer when and how many receipts are submitted in parallel, so the currently last commit should resolve your concerns with that too?
At the same time this change also reduces and limits the number of receipts we have "in-flight" which was the original intent behind this code.
|
CC: @#11319 |
This is a regression test for an issue identified in a review of near#11839
This is a regression test for an issue identified in a review of near#11839
|
@race-of-sloths include |
|
@nagisa Thank you for your contribution! Your pull request is now a part of the Race of Sloths! Current status: executed
Your contribution is much appreciated with a final score of 13! Congratulations @nagisa! Your PR was highly scored and you completed another monthly streak! To keep your monthly streak make another pull request next month and get 8+ score for it What is the Race of SlothsRace of Sloths is a friendly competition where you can participate in challenges and compete with other open-source contributors within your normal workflow For contributors:
For maintainers:
Feel free to check our website for additional details! Bot commands
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@race-of-sloths score 13 |
This is a regression test for an issue identified in a review of near#11839
This is a regression test for an issue identified in a review of near#11839
…ctions (#11897) During the development of #11839 an issue was identified that involved the first local receipt containing a deploy action. This PR adds a regression test for that issue. While here I also took an opportunity to move the test suite into its own module since the original file was getting… way too large.
This reverts commit 08cec9d.
This reverts commit 4e93e46.
This reverts commit 4e93e46.
This reverts commit 4e93e46.
This reverts commit 4e93e46.
This PR implements a very basic form of pipelining of the contract preparation. In particular with this PR only actions from one upcoming "interesting" local receipt is handled, with other receipt kinds (incoming, delayed) being left for a future change. I don't anticipate any trouble integrating them as they largely have similar data and structure as the local receipts seen here.
The primary reason I want this reviewed separately, however, is due to the changes to storage. You will notice that I have largely changed the code to read contracts without going through The State (i.e. it now reads the contract code directly via the code hash.) However at the same time writing of the contract continues involving The State; largely because I don't want to deal with migrations and similar such complexities… but also because
TrieStoragedoes not provide write access. This also means that writing is transactional and I had to add some work-arounds to thestore::contract::Storagein order to make deployed contracts visible before the commit.