-
Notifications
You must be signed in to change notification settings - Fork 143
[Compiler] Re-use static to sema conversions in vm. #4287
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
base: master
Are you sure you want to change the base?
Conversation
Benchstat comparison
Results
|
84d40f6
to
d6167e7
Compare
22c2edb
to
7bc4611
Compare
3c317a6
to
b7fad6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's surprising how/why caching doesn't improve the performance, given that first iteration of caching in #4054 gave us some improvement.
Given that we already cache it after the very first conversion, maybe that initial conversion overhead is trivial?
Could you please add some more stat: for example, the number of cache hits/misses before and after the improvement?
Additionally, do we also need to cache types coming from the imports / for the imported programs?
Variables []Variable[E] | ||
Types []T | ||
Globals []Global | ||
SemaTypeCache map[sema.TypeID]sema.Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bbq.Program
is currently serializable (and deserializable), even though we don't make use of it at the moment. But, we would lose this capability if we include sema.Type
information in bbq.Program
.
Could we instead pass this mapping externally? i.e: maybe as part of the vm.Config
?
Closes #4273
Description
Re-use static to sema conversions from the compiler in the vm. Tried various changes in the linker to no success, tried caching static type ID calls, which was an improvement but affects execution, so not included.
master
branchFiles changed
in the Github PR explorer