fix(gno.land): pre-load all standard libraries in vm.Initialize #2504
Conversation
github-actions
bot
added
📦 🤖 gnovm
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2504 +/- ##
==========================================
+ Coverage 54.70% 54.75% +0.04%
==========================================
Files 590 592 +2
Lines 79036 79150 +114
==========================================
+ Hits 43236 43337 +101
+ Misses 32550 32545 -5
- Partials 3250 3268 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
thehowl
requested review from
a team,
jaekwon,
moul,
piux2,
deelawn,
mvertes,
gfanton and
zivkovicmilos
as code owners
There was a problem hiding this comment.
Since you don't completely remove the support of packagegetter, and your fix mostly occurs in gno.land's vmkeepr, I favor merging this. Then, we can focus on speeding up local tests and take more time to fix the underlying caching issues that could still occur.
Pinging @jaekwon for a complementary review.
|
general approach seems good. |
|
I did it 😃 I implemented a cache mechanism, which keeps the writes to the iavlStore and the baseStore in a separated database, as well as an instance of the gno store, and then simply copies them after the first time. On my machine, this leads to a "cold" load of ~2.6 seconds, followed by hot loads of ~45 ms! As a consequence, the CI is now in line with the results on master :) if not some ~20 seconds faster (hard to tell, could be statistical error). In theory, it should be faster, as now any tests that import standard libraries won't have to spend time loading them on-the-fly; and can instead just use the cached data. |
…ang#2504) Fixes gnolang#2283 (with a different approach from gnolang#2319) This PR loads all the standard libraries into the database when vm.Initialize is called. It doesn't fully fix the problems that gnolang#2319 is trying to address, but it does fix the most immediate bug of not being able to publish certain packages on portal loop. With these changes, we don't need a PackageGetter on-chain anymore. All packages are already loaded into the store, thus solving the problem @jaekwon was talking about [here](gnolang#2319 (comment)) at its root. This PR has a problem, which is that adding loading the entire stdlibs in the VM initialization step brings a huge overhead computationally; this is not a problem on the node, but it is when testing as it needs to happen very often. This translates to 2x slower txtar tests, compared to master. On my PC, it adds a 2-3 second overhead whenever running Initialize. I tried working out on a system which could save the data to quickly recover it, at least for some cases where we need to Initialize often; [see this diff](https://gist.github.com/thehowl/cb1ee79e63cf77d3f323730580eb2d18). But I didn't get it to work so far; after copying the DB, attempting initialization crashes because [ParseMemPackage is being handed a nil package, for some reason](https://gist.github.com/thehowl/d1efa51858d865fb5beb9c3a9cb0eeef). @gfanton, any tips?? :)) I'd want to avoid lazy loading in the node, as that's what got us here in the first place. <details><summary>Contributors' checklist...</summary> - [ ] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [x] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Fixes #2283 (with a different approach from #2319)
This PR loads all the standard libraries into the database when vm.Initialize is called. It doesn't fully fix the problems that #2319 is trying to address, but it does fix the most immediate bug of not being able to publish certain packages on portal loop.
With these changes, we don't need a PackageGetter on-chain anymore. All packages are already loaded into the store, thus solving the problem @jaekwon was talking about here at its root.
This PR has a problem, which is that adding loading the entire stdlibs in the VM initialization step brings a huge overhead computationally; this is not a problem on the node, but it is when testing as it needs to happen very often. This translates to 2x slower txtar tests, compared to master. On my PC, it adds a 2-3 second overhead whenever running Initialize.
I tried working out on a system which could save the data to quickly recover it, at least for some cases where we need to Initialize often; see this diff. But I didn't get it to work so far; after copying the DB, attempting initialization crashes because ParseMemPackage is being handed a nil package, for some reason. @gfanton, any tips?? :)) I'd want to avoid lazy loading in the node, as that's what got us here in the first place.
Contributors' checklist...
BREAKING CHANGE: xxxmessage was included in the description