Revert "near-vm: use a recycling pool of shared code memories instead of a in-memory cache of loaded artifacts (#9244)" #10788
Conversation
nagisa
changed the title
| .map_err(|_| CacheError::DeserializationError)?; | ||
| let artifact = self | ||
|
|
||
| let compile_or_read_from_cache = || -> VMResult<Result<VMArtifact, CompilationError>> { |
There was a problem hiding this comment.
nit: best move to a dedicated method, both this closure and the compile_and_load are too long already
|
|
||
| let stored_artifact: Option<VMArtifact> = match cache_record { | ||
| None => None, | ||
| Some(CompiledContract::CompileModuleError(err)) => return Ok(Err(err)), |
There was a problem hiding this comment.
Ah yes, the "operation failed successfully" return :)
| #[cfg(feature = "no_cache")] | ||
| return compile_or_read_from_cache(); | ||
|
|
||
| #[cfg(not(feature = "no_cache"))] | ||
| return { |
There was a problem hiding this comment.
Would it be possible to do that on method level? It would be more readable.
There was a problem hiding this comment.
I would rather not modify this code at all given that it is legacy and there's no benefit to taking the risk IMO.
|
@nagisa Not sure what happened but it seems like some important checks failed. Can you check what is going on? e.g. cargo nextest (Linux) |
|
There's nothing much to look into here -- this PR/commit targets 1.38 branch and these branches just never pass the CI. |
|
I don't think that's the case but I had a look at the failures and it's just clippy and themis so not too bad. |
That's how it is today unfortunately. Before this PR the CI couldn't possibly pass due to justfile containing merge conflict markers, for example. But even then, themis could never pass because its checks actively conflict with the release procedure. And as a result nobody really looks at those checks. This is an unfortunate place to be, but this is where we are. cc @Ekleog-NEAR an idea for a CI related project -- make it enforced that the releases must pass the CI too :) The fact that we release of a branch that can't pass the tests (even if it wanted to) is a landmine waiting to be stepped on. |
|
Ah it must be a new thing, I fixed some old issue on the release branch about a year ago. I think it was sufficient to override the protocol voting date for CI. Yeah to be honest we should have green CI for release branch and if we can enforce that it would be even better. |
|
@nagisa Just sanity checking, will you be also merging it to master? |
|
I want to avoid the revert on master for this revert is speculative in how it might help and because we're working on optimizing contract loading in ways that are less unsavoury on top of this anyway.
----------------------------------------
15 Mar 2024 14:03:22 wacban ***@***.***>:
…
@nagisa[https://github.com/nagisa] Just sanity checking, will you be also merging it to master?
—
Reply to this email directly, view it on GitHub[#10788 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAFFZUTVUTXCDTVTNDX2RPTYYLPQRAVCNFSM6AAAAABEU5NPR6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJZGUYTKOJRGI].
You are receiving this because you were mentioned.
[Tracking image][https://github.com/notifications/beacon/AAFFZUWILKMLC6P4NZMER6TYYLPQRA5CNFSM6AAAAABEU5NPR6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTXFYYQQ.gif]
|
…d of a in-memory cache of loaded artifacts (near#9244)" (near#10788) This reverts commit bb02713.
This reverts commit ad67e6b.