Refactor external types to instantiate only once whenever possible #2225
Conversation
| func ExportType( | ||
| t sema.Type, | ||
| results map[sema.TypeID]cadence.Type, | ||
| ) cadence.Type { |
There was a problem hiding this comment.
This method was a duplicate of ExportMeteredType(nil, t, results)
Codecov Report
@@ Coverage Diff @@
## master #2225 +/- ##
==========================================
+ Coverage 77.77% 77.79% +0.01%
==========================================
Files 311 311
Lines 65970 65680 -290
==========================================
- Hits 51311 51097 -214
+ Misses 12889 12815 -74
+ Partials 1770 1768 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 559e775 Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Thanks for implementing PathLinkType and great idea to refactor simple types as singletons. 👍
I have a few questions:
- Should we support encoding and decoding
PathLinkTypeas type value? - Should we make new singleton functions such as
NewAnyType()unexported to encourage reusing singleton? - Should we use pointer for simple type singletons?
|
Thank you for having a look @fxamacker!
Currently, a PathLinkType cannot exist at Cadence runtime (there is no run-time/internal type counterpart for external PathLinkType). So even if we add encoding/decoding, it will not be used.
I also had the same thought, but eventually decided to keep it as is, so that any downstream dependencies (e.g: go-sdk, any community project, etc.) would not be broken by the API change. I did remove the metered constructor though, since it is supposed to be used internally only.
Yeah, the real use of singleton comes if we actually re-use the same memory location. But I'm not sure how will it impact perf (GC in particular). i.e: Go copying in stack vs GC-ing a heap object/pointer. Do you think a pointer would be better performant/memory efficient? |
SupunS
force-pushed
the
supun/external-types
branch
from
b014471 to
db15d9b
Compare
Ah, that makes sense. Maybe a comment can be added to document this.
Actually, escape analysis with go1.19.4 shows both non-pointer singleton and pointer singleton escape to the heap. Pointer singleton is automatically escaped to the heap. Non-pointer singleton is escaped because of how it is used by value object. So either approach is OK. var TheVoidType = NewVoidType()
func NewVoidType() *VoidType {
return &VoidType{}
}var TheVoidType = NewVoidType()
func NewVoidType() VoidType {
return VoidType{}
}
Another benefit of using singletons is memory optimization by reducing heap use. For example, the current implementation of func (Void) Type() Type {
return NewVoidType()
} |
|
I can also make the constructor return the singleton (instead of removing the constructor), so we can achieve both at once: reducing memory allocations while keeping the API backward compatible. var TheVoidType = VoidType{}
func NewVoidType() VoidType {
return TheVoidType
}
Oh, didn't know that! I was assuming otherwise. |
SupunS
force-pushed
the
supun/external-types
branch
from
18e9da0 to
5876405
Compare
Depends on: #2248
Description
New instances of Cadence external types are created every time during decoding, type conversions, etc.
However, some of them (simple types) can be instantiated only once and can be re-used, which also eliminates the need for memory metering. Hence refactored such types by introducing and re-using global instances.
Also noticed that
cadence.ExportTypemethod is a duplicate ofcadence.ExportMeteredTypewith the only difference being the memory gauge isnil. Refactored that as well, to re-use one method for both scenarios.masterbranchFiles changedin the Github PR explorer