Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(gnovm): test performance improvements #3210

Merged
merged 7 commits into from
Dec 2, 2024

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Nov 26, 2024

Fix master CI runs, and miscellaneous improvements locally, too.

  • ci: switch to using -covermode=set rather than atomic, as it significantly degrades performance while not being shown on codecov. more info
  • gnolang tests: use t.Parallel() to parallelize known "long" tests, both in -short and long versions.
  • stdlibs: provide unicode native shims for some common functions used in some standard library tests. This may lead to some small inconsistencies between on-chain behaviour and off-chain should the unicode packages diverge; but I think we might we might want to consider a native-based unicode stdlib, anyway.
  • thanks to these improvements, there is no longer the need to run -short on PRs, as the CI runs in ~9 mins, ie. 8 minutes less than the gno.land tests.

@thehowl thehowl self-assigned this Nov 26, 2024
@thehowl thehowl changed the title test: run long gnovm tests in parallel test(gnovm): run long tests in parallel Nov 26, 2024
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Nov 26, 2024
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 28, 2024
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 2, 2024

Merge Requirements

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🟢 Maintainers must be able to edit this pull request
🟢 The pull request head branch must be up-to-date with its base

Details
Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (thehowl:dev/morgan/long-parallel-tests) is up to date with base (master): behind by 0 / ahead by 7

Manual Checks

No manual checks match this pull request.

@thehowl
Copy link
Member Author

thehowl commented Dec 2, 2024

The tests are "slower" now because, putting stdlibs and filetests in /gnolang rather than /tests, code coverage is tracked more "correctly" (ie. filetests count towards the coverage), but running filetests and stdlib tests with coverage enabled on all pkg/gnolang has a big performance cost (-short -v on my machine):

ok      github.com/gnolang/gno/gnovm/pkg/gnolang        27.157s  # no coverage
ok      github.com/gnolang/gno/gnovm/pkg/gnolang        51.866s  # -covermode=set
ok      github.com/gnolang/gno/gnovm/pkg/gnolang        64.649s  # -covermode=count
ok      github.com/gnolang/gno/gnovm/pkg/gnolang        174.701s # -covermode=atomic

Currently, we are running with -covermode=atomic; I'm switching to -covermode=set, as codecov doesn't even allow to see the number of times a line was covered; so we can relegate analysis on specific lines to manual inspection when necessary.

@thehowl thehowl marked this pull request as ready for review December 2, 2024 13:01
@thehowl thehowl changed the title test(gnovm): run long tests in parallel test(gnovm): test performance improvements Dec 2, 2024
@thehowl thehowl requested review from mvertes and gfanton December 2, 2024 13:24
@thehowl
Copy link
Member Author

thehowl commented Dec 2, 2024

@gfanton, the ball is now onto you to lower the tests in gno.land down to ~10 mins ;)

Copy link
Contributor

@mvertes mvertes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@thehowl thehowl merged commit 43c9116 into gnolang:master Dec 2, 2024
106 of 108 checks passed
@thehowl thehowl deleted the dev/morgan/long-parallel-tests branch December 2, 2024 17:54
r3v4s pushed a commit to gnoswap-labs/gno that referenced this pull request Dec 10, 2024
Fix master CI runs, and miscellaneous improvements locally, too.

- ci: switch to using `-covermode=set` rather than atomic, as it
significantly degrades performance while not being shown on codecov.
[more
info](gnolang#3210 (comment))
- gnolang tests: use `t.Parallel()` to parallelize known "long" tests,
both in `-short` and long versions.
- stdlibs: provide `unicode` native shims for some common functions used
in some standard library tests. This may lead to some small
inconsistencies between on-chain behaviour and off-chain should the
`unicode` packages diverge; but I think we might we might want to
consider a native-based `unicode` stdlib, anyway.
- thanks to these improvements, there is no longer the need to run
`-short` on PRs, as the CI runs in ~9 mins, ie. 8 minutes less than the
gno.land tests.
@Kouteki Kouteki removed the in focus Core team is prioritizing this work label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Development

Successfully merging this pull request may close these issues.

5 participants