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
Optimize activations #571
Optimize activations #571
Conversation
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.
LGTM!
And nice work! Seeing over a 4x improvement in performance for checker+interpreter 💯
name := string(entry.(common.StringEntry)) | ||
return f(name, v) | ||
}) | ||
func (a *Activation) ForEach(cb func(string, interface{}) error) error { |
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.
This doesn't look like it iterates over the entries in the parent. This will be tricky, because I assume you don't want to iterate over any entires in a parent that are "shadowed" in a descendent. Not sure how to do this efficiently, to be honest!
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.
Refactored in 56e851c
(#571)
if current == nil { | ||
return NewActivation() | ||
return NewActivation(nil) |
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 to me that if there is no current activation, that you don't create one and make it the current. Clearly this logic hasn't changed with this PR, but maybe a comment is warranted here?
@@ -156,22 +156,21 @@ func (a *VariableActivations) DeclareImplicitConstant( | |||
) | |||
} | |||
|
|||
func (a *VariableActivations) VariablesDeclaredInAndBelow(depth int) map[string]*Variable { | |||
variables := map[string]*Variable{} | |||
func (a *VariableActivations) ForEachVariablesDeclaredInAndBelow(depth int, f func(name string, value *Variable)) { |
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.
Oh! I like this... Iterate in place instead of building a list and iterating over the list. 👍
Description
Replace the use of HAMT maps in activations with a plain map and a parent chain.
For contributor use:
master
branchFiles changed
in the Github PR explorer