feat: small improvements for #1702 #2276

jaekwon · 2024-06-04T19:17:20Z

Also includes improved coloredbytes and debug printing, and more comments.
At first I thought #1702's passing of the store was not ideal, but upon much confusion with cache invalidation,
it became clear that passing in a store to getPackage() makes sense.

This means that any store operations that occur through the loading of dependencies will incur gas charges for the transaction, e.g. for AddPkg() with dependencies like "time" or "strconv". Rather than clear cache-misses from the cacheStore, which is confusing, we would be better off passing in a mutated store go getPackage (if we need to).

Or, just load the standard packages upon genesis.

Also added improvements to ColoredBytes; this is now faster since not every character needs to be escaped, but rather escaping happens in chunks. As part of this refactor, the key & values are also clipped. I suppose we could maybe 1. improve ColoredBytesN() to clip exactly to N, but implementing this is non-trivial, and also 2. make the key/value limits perhps depend on a configuration or environment variable.

Poll... would you be sad if the Print() output for databases clipped the values? I think it makes it much better for dev experience; and if you need the full value you can tinker with the source where appropriate. The downside is, we might lose information from logs. But I'm not sure we even use the Print() feature for any logs as of now.

jaekwon · 2024-06-04T19:29:35Z

gno.land/pkg/sdk/vm/builtins.go

+			store.Print()
+			fmt.Println(colors.Red("VMKeeper.init.getPackage ========================================= before wrote end"))
+		}
+		store.Write() // XXX XXX XXX or flush?


essential change #1; the saving must be persisted to the underlying store, so that it can be picked up by a subsequent call that fetches it, but requires essential change #2 too

jaekwon · 2024-06-04T19:33:28Z

gnovm/pkg/gnolang/store.go

+				}
+				if cts, ok := ds.baseStore.(types.ClearThrougher); ok {
+					cts.ClearThrough()
+				}


essential change #2, since this transaction's CacheStore wrapper already cached the fact that the mempackage does not exist, we need to clear this non-dirty empty item from the cache in order to be able to fetch the mempackage that was written to the iavltree (see essential change #1).

codecov · 2024-06-04T19:34:36Z

Codecov Report

Attention: Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 54.72%. Comparing base (31a5f2e) to head (2f5589f).

Files	Patch %	Lines
gno.land/pkg/gnoland/app.go	0.00%	3 Missing ⚠️
gno.land/pkg/sdk/vm/builtins.go	86.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2276      +/-   ##
==========================================
+ Coverage   54.70%   54.72%   +0.02%     
==========================================
  Files         582      582              
  Lines       78430    78354      -76     
==========================================
- Hits        42902    42883      -19     
+ Misses      32315    32259      -56     
+ Partials     3213     3212       -1

Flag	Coverage Δ
contribs/gnodev	`23.81% <ø> (ø)`
contribs/gnofaucet	`15.31% <ø> (+0.85%)`	⬆️
contribs/gnokeykc	`0.00% <ø> (ø)`
contribs/gnomd	`0.00% <ø> (ø)`
gno.land	`62.02% <75.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jaekwon · 2024-06-04T20:49:07Z

gno.land/pkg/gnoland/app.go

-	vmKpr.Initialize(baseApp.GetCacheMultiStore())
+	ms := baseApp.GetCacheMultiStore()
+	vmKpr.Initialize(ms)
+	ms.MultiWrite() // XXX why was't this needed?


I think it was always requried, but its absence (of MultiWrite) being masked by the fact that later transactions would commit the same side effect, e.g. populate the mem package for dependencies like "time" or "strconv". But i'm not sure. If that's true, then loadpkg in genesis would have no effect for our integration tests?

Can somebody look into it?

jaekwon · 2024-06-04T20:50:41Z

gno.land/pkg/sdk/vm/builtins.go

@@ -16,7 +16,7 @@ func (vm *VMKeeper) initBuiltinPackagesAndTypes(store gno.Store) {
 	// NOTE: native functions/methods added here must be quick operations,


There's no real need to have this function at all... getPackage below might as well be a method on the store.

jaekwon · 2024-06-04T22:02:06Z

gno.land/pkg/sdk/vm/builtins.go

 	}
-	store.SetPackageGetter(getPackage)
-	store.SetNativeStore(stdlibs.NativeStore)


diff artifact, not actually removed from the original code block.

jaekwon · 2024-06-04T22:03:05Z

gnovm/tests/imports.go

+	resStore.SetPackageGetter(getPackage)
+	resStore.SetNativeStore(teststdlibs.NativeStore)
+	resStore.SetPackageInjector(testPackageInjector)
+	resStore.SetStrictGo2GnoMapping(false)


they're both new stores, so name neither "newStore" :)

jaekwon · 2024-06-04T22:13:58Z

gnovm/pkg/gnolang/store.go

@@ -86,7 +93,7 @@ type defaultStore struct {

 	// transient
 	opslog  []StoreOp // for debugging and testing.
-	current []string
+	current []string  // for detecting import cycles.


it's still detecting import cycles.

jaekwon · 2024-06-04T22:14:42Z

tm2/pkg/colors/colors.go

 		} else {
-			s += bytesColor(fmt.Sprintf("%02X", b))
+			s += bytesColor(buf)
+			buf = ""


If somebody can write a perfect n len-capped implementation that is simpler than this, I'd be impressed. But it's also probably unnecessary.

jaekwon · 2024-06-04T22:17:15Z

For reference, here's what DefaultColoredBytesN looks like when printing a tm2 Store
(notice the CacheStore above the base IAVLStore).

Also includes improved coloredbytes and debug printing, and more comments. At first I thought gnolang#1702's passing of the store was not ideal, but upon much confusion with cache invalidation, it became clear that passing in a store to getPackage() makes sense. This means that any store operations that occur through the loading of dependencies will incur gas charges for the transaction, e.g. for AddPkg() with dependencies like "time" or "strconv". Rather than clear cache-misses from the cacheStore, which is confusing, we would be better off passing in a mutated store go getPackage (if we need to). Or, just load the standard packages upon genesis. Also added improvements to ColoredBytes; this is now faster since not every character needs to be escaped, but rather escaping happens in chunks. As part of this refactor, the key & values are also clipped. I suppose we could maybe 1. improve ColoredBytesN() to clip exactly to N, but implementing this is non-trivial, and also 2. make the key/value limits perhps depend on a configuration or environment variable. Poll... would you be sad if the Print() output for databases clipped the values? I think it makes it much better for dev experience; and if you need the full value you can tinker with the source where appropriate. The downside is, we might lose information from logs. But I'm not sure we even use the Print() feature for any logs as of now.

jaekwon added 7 commits June 2, 2024 20:22

remove unnecessary store argument

7289a9a

...

1d4ef92

more debugging

c63ba9d

...

2b2212f

...

593db9a

...

4930964

most working

eed932e

jaekwon requested review from moul, piux2, deelawn, thehowl, mvertes, gfanton, zivkovicmilos, leohhhn, ajnavarro and a team as code owners June 4, 2024 19:17

github-actions bot assigned jaekwon Jun 4, 2024

github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jun 4, 2024

jaekwon commented Jun 4, 2024

View reviewed changes

revert cache invalidation stuff

d7134db

jaekwon commented Jun 4, 2024

View reviewed changes

jaekwon added 2 commits June 4, 2024 14:45

improve colored bytes (faster)

395e263

remove initBuiltinPackagesAndTypes

e454cf4

comment

d20ff79

jaekwon changed the title ~~WIP Dev/jae/go types typecheck~~ Small imrovements for #1702; improved coloredbytes and debug printing; more comments. Jun 4, 2024

jaekwon changed the title ~~Small imrovements for #1702; improved coloredbytes and debug printing; more comments.~~ Small imrovements for #1702 Jun 4, 2024

jaekwon commented Jun 4, 2024

View reviewed changes

remove spurious printer

b8e6373

jaekwon commented Jun 4, 2024

View reviewed changes

deelawn changed the title ~~Small imrovements for #1702~~ Small improvements for #1702 Jun 6, 2024

jaekwon added 2 commits June 23, 2024 19:32

Merge branch 'master' into dev/jae/go-types-typecheck

23208e1

lint

e16edff

jaekwon changed the title ~~Small improvements for #1702~~ feat: small improvements for #1702 Jun 24, 2024

bug fix

2f5589f

jaekwon merged commit 6243b56 into master Jun 24, 2024
93 checks passed

jaekwon deleted the dev/jae/go-types-typecheck branch June 24, 2024 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: small improvements for #1702 #2276

feat: small improvements for #1702 #2276

jaekwon commented Jun 4, 2024 •

edited

Loading

jaekwon Jun 4, 2024

jaekwon Jun 4, 2024

codecov bot commented Jun 4, 2024 •

edited

Loading

jaekwon Jun 4, 2024 •

edited

Loading

jaekwon Jun 4, 2024

jaekwon Jun 4, 2024

jaekwon Jun 4, 2024

jaekwon Jun 4, 2024

jaekwon Jun 4, 2024 •

edited

Loading

jaekwon commented Jun 4, 2024

		@@ -16,7 +16,7 @@ func (vm *VMKeeper) initBuiltinPackagesAndTypes(store gno.Store) {
		// NOTE: native functions/methods added here must be quick operations,

feat: small improvements for #1702 #2276

feat: small improvements for #1702 #2276

Conversation

jaekwon commented Jun 4, 2024 • edited Loading

jaekwon Jun 4, 2024

Choose a reason for hiding this comment

jaekwon Jun 4, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

jaekwon Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

jaekwon Jun 4, 2024

Choose a reason for hiding this comment

jaekwon Jun 4, 2024

Choose a reason for hiding this comment

jaekwon Jun 4, 2024

Choose a reason for hiding this comment

jaekwon Jun 4, 2024

Choose a reason for hiding this comment

jaekwon Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

jaekwon commented Jun 4, 2024

jaekwon commented Jun 4, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading

jaekwon Jun 4, 2024 •

edited

Loading

jaekwon Jun 4, 2024 •

edited

Loading