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

perf(gnovm/pkg/gnolang): make ReadMemPackageFromList singly use byteslices from os.ReadFile #3599

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

odeke-em
Copy link
Contributor

This change takes advantage of the fact that we can singly use the byteslices that were already allocated from os.ReadFile, given that the places that their string(bz) are invoked don't have any unsafe usages that then would modify those strings.

This change shaves off considerable CPU and RAM and is in the spirit of having a fast VM that can be relied on and not stymied per se by garbage collection. With it, all the RAM contributions for: PackageNameFromFile and gnovm.MemFile disappear shaving off 4+GB and no longer even appearing as a direct RAM contributor in the top 10.

$ benchstat before.txt after.txt
name              old time/op    new time/op    delta
ReadMemPackage-8     101µs ± 8%      89µs ± 4%  -11.54%  (p=0.000 n=9+10)

name              old alloc/op   new alloc/op   delta
ReadMemPackage-8    64.1kB ± 0%    34.2kB ± 0%  -46.72%  (p=0.001 n=8+9)

name              old allocs/op  new allocs/op  delta
ReadMemPackage-8      59.0 ± 0%      56.0 ± 0%   -5.08%  (p=0.000 n=10+10)

and then now RAM usage profiles show

Total: 7.41GB
ROUTINE ======================== github.com/gnolang/gno/gnovm/pkg/gnolang.ReadMemPackageFromList in /Users/emmanuelodeke/go/src/github.com/gnolang/gno/gnovm/pkg/gnolang/nodes.go
      34MB     6.98GB (flat, cum) 94.14% of Total
         .          .   1310:func ReadMemPackageFromList(list []string, pkgPath string) (*gnovm.MemPackage, error) {
   14.50MB    14.50MB   1311:	memPkg := &gnovm.MemPackage{Path: pkgPath}
         .          .   1312:	var pkgName Name
         .          .   1313:	for _, fpath := range list {
         .          .   1314:		fname := filepath.Base(fpath)
         .     4.35GB   1315:		bz, err := os.ReadFile(fpath)
         .          .   1316:		if err != nil {
         .          .   1317:			return nil, err
         .          .   1318:		}
         .          .   1319:		// XXX: should check that all pkg names are the same (else package is invalid)
         .          .   1320:		if pkgName == "" && strings.HasSuffix(fname, ".gno") {
         .          .   1321:			// The string derived from bz is never modified inside PackageNameFromFileBody
         .          .   1322:			// hence shave these unnecessary allocations by an unsafe
         .          .   1323:			// byteslice->string conversion per https://github.com/gnolang/gno/issues/3598
         .     2.59GB   1324:			pkgName, err = PackageNameFromFileBody(fname, unsafeBytesliceToStr(bz))
         .          .   1325:			if err != nil {
         .          .   1326:				return nil, err
         .          .   1327:			}
         .          .   1328:			if strings.HasSuffix(string(pkgName), "_test") {
         .          .   1329:				pkgName = pkgName[:len(pkgName)-len("_test")]
         .          .   1330:			}
         .          .   1331:		}
    6.50MB     6.50MB   1332:		memPkg.Files = append(memPkg.Files,
      13MB       13MB   1333:			&gnovm.MemFile{
         .          .   1334:				Name: fname,
         .          .   1335:				// The string derived from bz is never modified, hence to shave
         .          .   1336:				// unnecessary allocations, perform this unsafe byteslice->string
         .          .   1337:				// conversion per https://github.com/gnolang/gno/issues/3598
         .          .   1338:				// and the Go garbage collector will knows to garbage collect
         .          .   1339:				// bz when no other object points to that memory.
         .          .   1340:				Body: unsafeBytesliceToStr(bz),
         .          .   1341:			})
         .          .   1342:	}
         .          .   1343:
         .          .   1344:	// If no .gno files are present, package simply does not exist.
         .          .   1345:	if !memPkg.IsEmpty() {
         .     7.01MB   1346:		if err := validatePkgName(string(pkgName)); err != nil {
         .          .   1347:			return nil, err
         .          .   1348:		}
         .          .   1349:		memPkg.Name = string(pkgName)
         .          .   1350:	}

but previously looked like

ROUTINE ======================== github.com/gnolang/gno/gnovm/pkg/gnolang.ReadMemPackageFromList in /Users/emmanuelodeke/go/src/github.com/gnolang/gno/gnovm/pkg/gnolang/nodes.go
    5.55GB    11.46GB (flat, cum) 97.01% of Total
         .          .   1309:func ReadMemPackageFromList(list []string, pkgPath string) (*gnovm.MemPackage, error) {
      13MB       13MB   1310:	memPkg := &gnovm.MemPackage{Path: pkgPath}
         .          .   1311:	var pkgName Name
         .          .   1312:	for _, fpath := range list {
         .          .   1313:		fname := filepath.Base(fpath)
         .     3.66GB   1314:		bz, err := os.ReadFile(fpath)
         .          .   1315:		if err != nil {
         .          .   1316:			return nil, err
         .          .   1317:		}
         .          .   1318:		// XXX: should check that all pkg names are the same (else package is invalid)
         .          .   1319:		if pkgName == "" && strings.HasSuffix(fname, ".gno") {
    2.02GB     4.27GB   1320:			pkgName, err = PackageNameFromFileBody(fname, string(bz))
         .          .   1321:			if err != nil {
         .          .   1322:				return nil, err
         .          .   1323:			}
         .          .   1324:			if strings.HasSuffix(string(pkgName), "_test") {
         .          .   1325:				pkgName = pkgName[:len(pkgName)-len("_test")]
         .          .   1326:			}
         .          .   1327:		}
    4.50MB     4.50MB   1328:		memPkg.Files = append(memPkg.Files,
    9.50MB     9.50MB   1329:			&gnovm.MemFile{
         .          .   1330:				Name: fname,
    3.50GB     3.50GB   1331:				Body: string(bz),
         .          .   1332:			})
         .          .   1333:	}
         .          .   1334:
         .          .   1335:	// If no .gno files are present, package simply does not exist.
         .          .   1336:	if !memPkg.IsEmpty() {
         .     6.51MB   1337:		if err := validatePkgName(string(pkgName)); err != nil {
         .          .   1338:			return nil, err
         .          .   1339:		}
         .          .   1340:		memPkg.Name = string(pkgName)
         .          .   1341:	}

Updates #3435
Fixes #3598

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Jan 23, 2025
@Gno2D2 Gno2D2 requested a review from a team January 23, 2025 19:25
@Gno2D2 Gno2D2 added the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Jan 23, 2025
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Jan 23, 2025

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
  • The pull request description provides enough details (checked by @n2p5)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Pending initial approval by a review team member, or review from tech-staff

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: odeke-em/gno)

Then

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

Pending initial approval by a review team member, or review from tech-staff

If

🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: tech-staff)

Then

🟢 Requirement satisfied
└── 🟢 If
    ├── 🟢 Condition
    │   └── 🟢 Or
    │       ├── 🔴 At least 1 user(s) of the organization reviewed the pull request (with state "APPROVED")
    │       ├── 🟢 At least 1 user(s) of the team tech-staff reviewed pull request
    │       └── 🔴 This pull request is a draft
    └── 🟢 Then
        └── 🟢 Not (🔴 This label is applied to pull request: review/triage-pending)

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)
    └── 🟢 Not (🔴 Pull request author is user: dependabot[bot])

Can be checked by

  • team core-contributors

@odeke-em odeke-em force-pushed the optimize-gnolang.ReadMemPackage+other-fixes branch from 926bf9f to b4ea83a Compare January 23, 2025 19:29
@n2p5 n2p5 requested review from moul, thehowl and n0izn0iz January 23, 2025 19:35
Copy link
Contributor

@n2p5 n2p5 left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm only reviewing as a Comment, since more folks from the gno-core team should look at this first.

@Gno2D2 Gno2D2 removed the review/triage-pending PRs opened by external contributors that are waiting for the 1st review label Jan 23, 2025
…lices from os.ReadFile

This change takes advantage of the fact that we can singly use the
byteslices that were already allocated from os.ReadFile, given that
the places that their string(bz) are invoked don't have any unsafe
usages that then would modify those strings.

This change shaves off considerable CPU and RAM and is in the spirit
of having a fast VM that can be relied on and not stymied per se by
garbage collection. With it, all the RAM contributions for:
PackageNameFromFile and gnovm.MemFile disappear shaving off 4+GB
and no longer even appearing as a direct RAM contributor in the top 10.

```shell
$ benchstat before.txt after.txt
name              old time/op    new time/op    delta
ReadMemPackage-8     101µs ± 8%      89µs ± 4%  -11.54%  (p=0.000 n=9+10)

name              old alloc/op   new alloc/op   delta
ReadMemPackage-8    64.1kB ± 0%    34.2kB ± 0%  -46.72%  (p=0.001 n=8+9)

name              old allocs/op  new allocs/op  delta
ReadMemPackage-8      59.0 ± 0%      56.0 ± 0%   -5.08%  (p=0.000 n=10+10)
```

and then now RAM usage profiles show

```shell
Total: 7.41GB
ROUTINE ======================== github.com/gnolang/gno/gnovm/pkg/gnolang.ReadMemPackageFromList in /Users/emmanuelodeke/go/src/github.com/gnolang/gno/gnovm/pkg/gnolang/nodes.go
      34MB     6.98GB (flat, cum) 94.14% of Total
         .          .   1310:func ReadMemPackageFromList(list []string, pkgPath string) (*gnovm.MemPackage, error) {
   14.50MB    14.50MB   1311:	memPkg := &gnovm.MemPackage{Path: pkgPath}
         .          .   1312:	var pkgName Name
         .          .   1313:	for _, fpath := range list {
         .          .   1314:		fname := filepath.Base(fpath)
         .     4.35GB   1315:		bz, err := os.ReadFile(fpath)
         .          .   1316:		if err != nil {
         .          .   1317:			return nil, err
         .          .   1318:		}
         .          .   1319:		// XXX: should check that all pkg names are the same (else package is invalid)
         .          .   1320:		if pkgName == "" && strings.HasSuffix(fname, ".gno") {
         .          .   1321:			// The string derived from bz is never modified inside PackageNameFromFileBody
         .          .   1322:			// hence shave these unnecessary allocations by an unsafe
         .          .   1323:			// byteslice->string conversion per gnolang#3598
         .     2.59GB   1324:			pkgName, err = PackageNameFromFileBody(fname, unsafeBytesliceToStr(bz))
         .          .   1325:			if err != nil {
         .          .   1326:				return nil, err
         .          .   1327:			}
         .          .   1328:			if strings.HasSuffix(string(pkgName), "_test") {
         .          .   1329:				pkgName = pkgName[:len(pkgName)-len("_test")]
         .          .   1330:			}
         .          .   1331:		}
    6.50MB     6.50MB   1332:		memPkg.Files = append(memPkg.Files,
      13MB       13MB   1333:			&gnovm.MemFile{
         .          .   1334:				Name: fname,
         .          .   1335:				// The string derived from bz is never modified, hence to shave
         .          .   1336:				// unnecessary allocations, perform this unsafe byteslice->string
         .          .   1337:				// conversion per gnolang#3598
         .          .   1338:				// and the Go garbage collector will knows to garbage collect
         .          .   1339:				// bz when no other object points to that memory.
         .          .   1340:				Body: unsafeBytesliceToStr(bz),
         .          .   1341:			})
         .          .   1342:	}
         .          .   1343:
         .          .   1344:	// If no .gno files are present, package simply does not exist.
         .          .   1345:	if !memPkg.IsEmpty() {
         .     7.01MB   1346:		if err := validatePkgName(string(pkgName)); err != nil {
         .          .   1347:			return nil, err
         .          .   1348:		}
         .          .   1349:		memPkg.Name = string(pkgName)
         .          .   1350:	}
```

but previously looked like

```shell
ROUTINE ======================== github.com/gnolang/gno/gnovm/pkg/gnolang.ReadMemPackageFromList in /Users/emmanuelodeke/go/src/github.com/gnolang/gno/gnovm/pkg/gnolang/nodes.go
    5.55GB    11.46GB (flat, cum) 97.01% of Total
         .          .   1309:func ReadMemPackageFromList(list []string, pkgPath string) (*gnovm.MemPackage, error) {
      13MB       13MB   1310:	memPkg := &gnovm.MemPackage{Path: pkgPath}
         .          .   1311:	var pkgName Name
         .          .   1312:	for _, fpath := range list {
         .          .   1313:		fname := filepath.Base(fpath)
         .     3.66GB   1314:		bz, err := os.ReadFile(fpath)
         .          .   1315:		if err != nil {
         .          .   1316:			return nil, err
         .          .   1317:		}
         .          .   1318:		// XXX: should check that all pkg names are the same (else package is invalid)
         .          .   1319:		if pkgName == "" && strings.HasSuffix(fname, ".gno") {
    2.02GB     4.27GB   1320:			pkgName, err = PackageNameFromFileBody(fname, string(bz))
         .          .   1321:			if err != nil {
         .          .   1322:				return nil, err
         .          .   1323:			}
         .          .   1324:			if strings.HasSuffix(string(pkgName), "_test") {
         .          .   1325:				pkgName = pkgName[:len(pkgName)-len("_test")]
         .          .   1326:			}
         .          .   1327:		}
    4.50MB     4.50MB   1328:		memPkg.Files = append(memPkg.Files,
    9.50MB     9.50MB   1329:			&gnovm.MemFile{
         .          .   1330:				Name: fname,
    3.50GB     3.50GB   1331:				Body: string(bz),
         .          .   1332:			})
         .          .   1333:	}
         .          .   1334:
         .          .   1335:	// If no .gno files are present, package simply does not exist.
         .          .   1336:	if !memPkg.IsEmpty() {
         .     6.51MB   1337:		if err := validatePkgName(string(pkgName)); err != nil {
         .          .   1338:			return nil, err
         .          .   1339:		}
         .          .   1340:		memPkg.Name = string(pkgName)
         .          .   1341:	}
```

Updates gnolang#3435
Fixes gnolang#3598
@odeke-em odeke-em force-pushed the optimize-gnolang.ReadMemPackage+other-fixes branch from b4ea83a to 66f1d97 Compare January 24, 2025 07:34
Copy link

codecov bot commented Jan 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

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
Status: Triage
3 participants