perf: make tests run faster #1417

petar-dambovaliev · 2023-12-07T04:45:11Z

This PR enables running tests in parallel by making the Machine data structure copy-able and thus threadsafe.

DeepCopy methods added to all types that are contained within the Machine. including types that implement interfaces Expr, Object, Store and Type.

Before each test is launched, the Machine object is deep copied and passed along so that test can read and write on its own instance.
Global debugging functionality, including the accumulation of errors was moved into local scopes.
the var preprocessing int counter was moved into its own type with the relevant methods and its passed as an argument
Debugging is a field of every struct that needs to use it (i tried adding it as a method argument first, it didn't end well. The code was becoming extremely noisy)

codecov · 2023-12-07T04:49:50Z

Codecov Report

Attention: 2467 lines in your changes are missing coverage. Please review.

Comparison is base (c3a4993) 55.85% compared to head (aa13f3f) 55.23%.

Files	Patch %	Lines
gnovm/pkg/gnolang/op_binary.go	29.30%	408 Missing and 19 partials ⚠️
gnovm/pkg/gnolang/values.go	11.06%	358 Missing and 36 partials ⚠️
gnovm/pkg/gnolang/nodes.go	1.31%	294 Missing and 6 partials ⚠️
gnovm/pkg/gnolang/types.go	24.87%	291 Missing and 8 partials ⚠️
gnovm/pkg/gnolang/machine.go	4.14%	136 Missing and 26 partials ⚠️
gnovm/pkg/gnolang/gonative.go	30.27%	140 Missing and 12 partials ⚠️
gnovm/pkg/gnolang/op_inc_dec.go	16.32%	80 Missing and 2 partials ⚠️
gnovm/pkg/gnolang/realm.go	33.05%	62 Missing and 19 partials ⚠️
gnovm/pkg/gnolang/nodes_copy.go	0.00%	72 Missing ⚠️
gnovm/pkg/gnolang/op_unary.go	25.80%	65 Missing and 4 partials ⚠️
... and 23 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1417      +/-   ##
==========================================
- Coverage   55.85%   55.23%   -0.62%     
==========================================
  Files         431      432       +1     
  Lines       65729    67634    +1905     
==========================================
+ Hits        36713    37360     +647     
- Misses      26140    27375    +1235     
- Partials     2876     2899      +23

Flag	Coverage Δ
go-1.21.x	`∅ <ø> (∅)`
misc	`∅ <ø> (∅)`
misc-_test.genstd	`∅ <ø> (∅)`

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.

thehowl · 2024-01-07T21:15:48Z

@petar-dambovaliev ready for review?

I'll try to take a look at it this week. (It's close to the top of my review-list, so should be able to take a look at it, but no guarantees).

Thanks for the hard work.

zivkovicmilos · 2024-01-08T11:50:30Z

@petar-dambovaliev ready for review?

I'll try to take a look at it this week. (It's close to the top of my review-list, so should be able to take a look at it, but no guarantees).

Thanks for the hard work.

Should be ready for viewing as far as I know

gnovm/pkg/gnolang/debug.go

zivkovicmilos

Ran this locally, seems to shave ~1min on my machine for make test in the gnovm subfolder 🎉

Thank you for removing globals. I see this PR as a good start 👍

ajnavarro

doing a partial review, GitHub is failing to load the diff.

ajnavarro · 2024-01-22T10:29:07Z

gnovm/pkg/gnolang/debug.go

+func (d *Debugging) DeepCopy() *Debugging {
+	if d == nil {
+		return nil
+	}

-var enabled bool = true
+	deers := make([]string, len(d.derrors))

-func (d debugging) Println(args ...interface{}) {
-	if d {
-		if enabled {
-			fmt.Println(append([]interface{}{"DEBUG:"}, args...)...)
-		}
+	copy(deers, d.derrors)
+
+	return &Debugging{
+		enabled: d.enabled,
+		derrors: deers,
 	}
 }


why is this needed?

Agreed with @ajnavarro. Because we're only appending to d.derrors (never changing items, only appending or setting it directly to nil) it's actually safe to return derrors directly.

ajnavarro · 2024-01-22T10:32:00Z

gnovm/pkg/gnolang/debug.go

+	if d == nil {
+		return nil
+	}


why check d is nill? this might swallow implementation errors.

ajnavarro · 2024-01-22T10:32:50Z

gnovm/pkg/gnolang/debug.go

-func IsDebug() bool {
-	return bool(debug)
+func (d *Debugging) IsDebug() bool {
+	return d != nil


Suggested change

return d != nil

return d.enabled

ajnavarro · 2024-01-22T10:34:12Z

gnovm/pkg/gnolang/debug.go

+func (d *Debugging) DisableDebug() {
+	if d != nil {
+		d.enabled = false
+	}
 }

-func EnableDebug() {
-	enabled = true
+func (d *Debugging) EnableDebug() {
+	if d != nil {
+		d.enabled = true
+	}
 }


Instead of disable/enable debug, why not just SetEnabled(bool)?

ajnavarro · 2024-01-22T10:39:53Z

gnovm/pkg/gnolang/debug.go

-// using a const is probably faster.
-// const debug debugging = true // or flip
-var debug debugging = false
+type Debugging struct {


Why instead of passing it around, don't we just use the singleton pattern used in go log implementations?

var Log = &debugging{enabled:true} type debugging struct { ... } [...]

In any case, it must be thread-safe.

thehowl

So, I had a feeling that adding new pointers to all of these structs may actually not be the best thing for performance. The tests may run faster, but overall execution is actually slower:

goos: linux
goarch: amd64
pkg: github.com/gnolang/gno/gnovm/pkg/gnolang
cpu: AMD Ryzen 7 PRO 4750U with Radeon Graphics
                                │ bench_master.txt │           bench_1406.txt            │
                                │      sec/op      │   sec/op     vs base                │
Benchdata/fib.gno_param:4-16          32.27µ ±  5%   32.47µ ± 7%        ~ (p=0.393 n=10)
Benchdata/fib.gno_param:8-16          228.8µ ±  5%   236.0µ ± 1%   +3.15% (p=0.000 n=10)
Benchdata/fib.gno_param:16-16         11.02m ±  4%   10.91m ± 5%        ~ (p=0.481 n=10)
Benchdata/loop.gno-16                 493.3n ± 11%   698.5n ± 6%  +41.60% (p=0.000 n=10)
Benchdata/matrix.gno_param:3-16       653.9µ ±  4%   707.7µ ± 6%   +8.22% (p=0.004 n=10)
Benchdata/matrix.gno_param:4-16       1.862m ±  3%   2.081m ± 4%  +11.74% (p=0.000 n=10)
Benchdata/matrix.gno_param:5-16       6.822m ±  6%   7.643m ± 5%  +12.04% (p=0.000 n=10)
Benchdata/matrix.gno_param:6-16       34.66m ±  5%   38.16m ± 5%  +10.11% (p=0.000 n=10)
geomean                               572.6µ         630.8µ       +10.17%

go test -v -bench 'Benchdata' -run 'NONE' -count 10 | tee ~/throw/bench_1406.txt

These are tests done with the benchmarking system I added in #1624 (note it specifically targets benchmarking execution, not preprocessing) and as you can see there is an average 10% more time taken on each operation.

I think this makes clear our need for good benchmarking and proves the point of setting this up in #1624. Please, if you see problems with the methodology, or see a benchmark which disproves these benchmarks, do point them out. I pushed the branch with this branch + the changes in #1624 in 1406-with-bench

I think there are some foundational ideas in this PR which are good, some which need to be iterated before we can have tests which are both parallelized but don't sacrifice execution-time performance.

Thanks!

thehowl · 2024-02-02T16:20:34Z

gnovm/Makefile

@@ -39,7 +39,7 @@ fmt:
 .PHONY: test
 test: _test.cmd _test.pkg _test.gnolang

-GOTEST_FLAGS ?= -v -p 1 -timeout=30m
+GOTEST_FLAGS ?= -timeout=30m


I get removing -p (also because reading the docs looks like it wasn't all that necessary) but why remove -v?

It is giving out a lot of spam. You generally don't care for verbose when running all tests.
You care about verbose when running a single test.

thehowl · 2024-02-02T16:48:04Z

gnovm/pkg/gnolang/nodes_string.go

@@ -123,7 +123,7 @@ func (x IndexExpr) String() string {
 }

 func (x SelectorExpr) String() string {
-	// NOTE: for debugging selector issues:
+	// NOTE: for  selector issues:


intellij find and replace

thehowl · 2024-02-02T16:50:45Z

gnovm/pkg/gnolang/debug.go

-// using a const is probably faster.
-// const debug debugging = true // or flip
-var debug debugging = false
+type Debugging struct {


Please add type documentation

thehowl · 2024-02-02T16:52:33Z

gnovm/pkg/gnolang/debug.go

-// const debug debugging = true // or flip
-var debug debugging = false
+type Debugging struct {
+	enabled bool


If we have a Debugging type and it's passed as a pointer, would its mere existance not mean that it is supposed to be enabled?

We could keep things as they are as well. Ie. m.Debugging.Println, func (d *Debugging) Println checks that d != nil before doing so...

gnovm/pkg/gnolang/debug.go

thehowl · 2024-02-02T16:54:42Z

gnovm/pkg/gnolang/debug.go

+func (d *Debugging) DeepCopy() *Debugging {
+	if d == nil {
+		return nil
+	}

-var enabled bool = true
+	deers := make([]string, len(d.derrors))

-func (d debugging) Println(args ...interface{}) {
-	if d {
-		if enabled {
-			fmt.Println(append([]interface{}{"DEBUG:"}, args...)...)
-		}
+	copy(deers, d.derrors)
+
+	return &Debugging{
+		enabled: d.enabled,
+		derrors: deers,
 	}
 }


Agreed with @ajnavarro. Because we're only appending to d.derrors (never changing items, only appending or setting it directly to nil) it's actually safe to return derrors directly.

thehowl · 2024-02-02T16:59:53Z

gnovm/pkg/gnolang/debug.go

So, summary of what I think about the way you modified this:

I think we should simplify "enabled" to d != nil. If at one point we want to disable debugging, since we're passing it around as function parameters and in machines, we just set it to nil where necessary. Removes the necessity for functions Enable/DisableDebug.

Furthermore, I think it makes sense to create, together with this, an option to disable debugging at compile-time entirely. This can be a simple constant, maybe in an internal/ package if we don't want to pollute the gnolang directory even further, with the two following files/variations:

//go:build !nodebug package internal const DebuggingEnabled = true

//go:build nodebug package internal const DebuggingEnabled = false

At this point we need to check DebuggingEnabled in each function here (can alias it, too). Since it's a constant, the compiler can see it and remove the entire function body, and probably inline a lot of functions using debugging along the way.

thehowl · 2024-02-02T17:28:59Z

gnovm/pkg/gnolang/values.go

So, I don't think we necessarily need what @ajnavarro mentioned (ie. making debugging a singleton like it was before the PR), but I do think we need to split its two uses, which actually completely avoids what happened in this file: that every file type now gets the debugging variable.

I suggest that for all cases where the debugging variable guards an additional check like the vast majority of the ones here, that we instead opt for the same solution I'm proposing to disable debugging at compile time: build tags. These are all checks which provide "additional safety", so I would have a value like const Safety = true, enabled by default, that can be disabled with build tag -tags optimize or -tags nosafety.

thehowl

Sorry, meant to indicate request changes.

petar-dambovaliev · 2024-02-22T14:05:45Z

@thehowl

#1417 (comment)
2 threads appending to a slice without a synchronization mechanism is a concurrency violation.

save

4196342

petar-dambovaliev requested review from jaekwon, moul and a team as code owners December 7, 2023 04:45

petar-dambovaliev marked this pull request as draft December 7, 2023 04:45

github-actions bot assigned petar-dambovaliev Dec 7, 2023

github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Dec 7, 2023

save

14bd0ea

petar-dambovaliev force-pushed the 1406 branch from 3883e1b to 14bd0ea Compare December 7, 2023 19:07

petar-dambovaliev changed the title ~~make tests run faster~~ perf: make tests run faster Dec 7, 2023

save

4b9d3a3

michelleellen mentioned this pull request Dec 13, 2023

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

petar-dambovaliev added 5 commits December 13, 2023 16:01

save

89caaff

save

d188387

save

7026ffd

save

8cb29b5

save

fc43a42

zivkovicmilos linked an issue Dec 22, 2023 that may be closed by this pull request

Significantly long GnoVM test runtime #1406

Open

petar-dambovaliev added 2 commits December 22, 2023 13:54

save

5e1b162

save

c3b802a

zivkovicmilos self-requested a review December 24, 2023 11:23

petar-dambovaliev added 4 commits December 24, 2023 20:31

save

ebf5047

save

6797f24

save

84f1e2d

save

30c3ee2

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Dec 25, 2023

petar-dambovaliev added 3 commits December 27, 2023 15:20

save

864e71e

save

dc68816

save

5803876

petar-dambovaliev added 3 commits December 30, 2023 03:13

save

2318062

save

9a9ffb1

save

aa13f3f

petar-dambovaliev marked this pull request as ready for review December 30, 2023 14:13

harry-hov self-requested a review January 8, 2024 12:07

zivkovicmilos reviewed Jan 16, 2024

View reviewed changes

gnovm/pkg/gnolang/debug.go Show resolved Hide resolved

zivkovicmilos approved these changes Jan 16, 2024

View reviewed changes

zivkovicmilos requested review from thehowl and mvertes January 16, 2024 16:10

ajnavarro reviewed Jan 22, 2024

View reviewed changes

zivkovicmilos mentioned this pull request Jan 29, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

thehowl reviewed Feb 2, 2024

View reviewed changes

thehowl requested changes Feb 2, 2024

View reviewed changes

zivkovicmilos added this to the 🏗4️⃣ test4.gno.land milestone Mar 25, 2024

petar-dambovaliev closed this May 15, 2024

zivkovicmilos removed a link to an issue May 15, 2024

Significantly long GnoVM test runtime #1406

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: make tests run faster #1417

perf: make tests run faster #1417

petar-dambovaliev commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

thehowl commented Jan 7, 2024

zivkovicmilos commented Jan 8, 2024

zivkovicmilos left a comment

ajnavarro left a comment

ajnavarro Jan 22, 2024

thehowl Feb 2, 2024

ajnavarro Jan 22, 2024

ajnavarro Jan 22, 2024

ajnavarro Jan 22, 2024

ajnavarro Jan 22, 2024

thehowl left a comment

thehowl Feb 2, 2024

petar-dambovaliev Feb 22, 2024

thehowl Feb 2, 2024

petar-dambovaliev Feb 22, 2024

thehowl Feb 2, 2024

thehowl Feb 2, 2024

thehowl Feb 2, 2024

thehowl Feb 2, 2024

thehowl Feb 2, 2024 •

edited

Loading

thehowl left a comment

petar-dambovaliev commented Feb 22, 2024 •

edited

Loading

perf: make tests run faster #1417

perf: make tests run faster #1417

Conversation

petar-dambovaliev commented Dec 7, 2023 • edited Loading

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

thehowl commented Jan 7, 2024

zivkovicmilos commented Jan 8, 2024

zivkovicmilos left a comment

Choose a reason for hiding this comment

ajnavarro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehowl Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

thehowl left a comment

Choose a reason for hiding this comment

petar-dambovaliev commented Feb 22, 2024 • edited Loading

petar-dambovaliev commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

thehowl Feb 2, 2024 •

edited

Loading

petar-dambovaliev commented Feb 22, 2024 •

edited

Loading