perf(gnovm): cache PkgIDFromPkgPath for higher performance #3424
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
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) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
@n0izn0iz I considered using a small slice say pre-allocated 100 items as the cache then linear looking up but that wasn't much different. -var pkgIDFromPkgPathCache = make(map[string]*PkgID)
+type pkgCached struct {
+ path string
+ id *PkgID
+}
+
+var pkgIDFromPkgPathCache = make([]*pkgCached, 0, 100)
func PkgIDFromPkgPath(path string) PkgID {
pkgIDMu.Lock()
defer pkgIDMu.Unlock()
- pkgID, ok := pkgIDFromPkgPathCache[path]
- if !ok {
- pkgID = new(PkgID)
- *pkgID = PkgID{HashBytes([]byte(path))}
- pkgIDFromPkgPathCache[path] = pkgID
+ // Find the entry if possible, using a slice as
+ // we don't expect too many unique packages and
+ // when small, looking up with a slice is much
+ // cheaper than using a map.
+ for _, cached := range pkgIDFromPkgPathCache {
+ if cached.path == path {
+ return *cached.id
+ }
+ }
+
+ // Now cache the value.
+ cached := &pkgCached{
+ id: PkgID{HashBytes([]byte(path))},
+ path: path,
}
- return *pkgID
+ pkgIDFromPkgPathCache = append(pkgIDFromPkgPathCache, cached)
+ return *cached.id
}A map suffices TBH even if memory increases, it is unlikely that 5,000 packages will be imported and thus we can safely assume memory bloat won't be a problem. The merits of the change to me and in experience massively outweight any minuite RAM increases: being able to even start with 8-10 seconds less allows every contributor to work much faster, and even for Gno to preserve compute resources :-) |
odeke-em
force-pushed
the
gnovm-cache-PkgIDFromPkgPath-results
branch
2 times, most recently
from
5112853 to
13b78e9
Compare
This change comes from noticing that PkgIDFromPkgPath is very
heavily invoked within the VM yet its results with the same inputs
produced deterministic results aka SHA256(path)[:20]
Previously just spinning up the VM would take 80 seconds, with this
change that's shaved by ~8-10 seconds down and with repeatable and
visible results exhibited through profiles:
* Before
```shell
(pprof) list PkgIDFromPkgPath
Total: 100.94s
ROUTINE ======================== github.com/gnolang/gno/gnovm/pkg/gnolang.PkgIDFromPkgPath in $PATH
220ms 9.26s (flat, cum) 9.17% of Total
. . 74:func PkgIDFromPkgPath(path string) PkgID {
220ms 9.26s 75: return PkgID{HashBytes([]byte(path))}
. . 76:}
. . 77:
. . 78:// Returns the ObjectID of the PackageValue associated with path.
. . 79:func ObjectIDFromPkgPath(path string) ObjectID {
. . 80: pkgID := PkgIDFromPkgPath(path)
```
* After
```shell
(pprof) list PkgIDFromPkgPath
Total: 93.22s
ROUTINE ======================== github.com/gnolang/gno/gnovm/pkg/gnolang.PkgIDFromPkgPath in $PATH
210ms 1.55s (flat, cum) 1.66% of Total
50ms 50ms 78:func PkgIDFromPkgPath(path string) PkgID {
. 490ms 79: pkgIDMu.Lock()
10ms 10ms 80: defer pkgIDMu.Unlock()
. . 81:
10ms 730ms 82: pkgID, ok := pkgIDFromPkgPathCache[path]
. . 83: if !ok {
. . 84: pkgID = new(PkgID)
. . 85: *pkgID = PkgID{HashBytes([]byte(path))}
. . 86: pkgIDFromPkgPathCache[path] = pkgID
. . 87: }
140ms 270ms 88: return *pkgID
. . 89:}
. . 90:
. . 91:// Returns the ObjectID of the PackageValue associated with path.
. . 92:func ObjectIDFromPkgPath(path string) ObjectID {
. . 93: pkgID := PkgIDFromPkgPath(path)
```
Fixes gnolang#3423
odeke-em
force-pushed
the
gnovm-cache-PkgIDFromPkgPath-results
branch
from
13b78e9 to
b88c690
Compare
There was a problem hiding this comment.
That looks reasonable. However, an LRU cache would prevent it from causing a DoS. I'm fine with merging now since this DoS is unlikely to occur before other issues arise, but please add a TODO comment.
It also needs a review from @thehowl, @ltzmaxwell, and @petar-dambovaliev.
|
Done, thanks @moul I've added the TODO about using an LRU for the future when the trade-off is needed. |
This change comes from noticing that PkgIDFromPkgPath is very heavily invoked within the VM yet its results with the same inputs produced deterministic results aka SHA256(path)[:20] Previously just spinning up the VM would take 80 seconds, with this change that's shaved by ~8-10 seconds down and with repeatable and visible results exhibited through profiles:
Fixes #3423