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

get_parent() is excruciatingly slow #1024

Open
clinssen opened this issue Apr 16, 2024 · 0 comments · May be fixed by #1049
Open

get_parent() is excruciatingly slow #1024

clinssen opened this issue Apr 16, 2024 · 0 comments · May be fixed by #1049

Comments

@clinssen
Copy link
Contributor

There is a context condition check on the NESTML built-in resolution() function, which needs to find the block that each resolution() function call is enclosed in: the update block, equations, parameters, etc. (In the equations block and in functions, it is not allowed.) To check this coco, there is a visitor which finds each resolution() function call, and then tries to find the parent block of the call by calling ast_neuron.get_parent(ast_resolution_func_call). It turns out that get_parent() is implemented by hand for each AST node, and does a depth-first search on the entire ast_neuron to find ast_resolution_func_call. This works, in principle. However, we now have a model that has several resolution() function calls, which is resulting in tens of millions of calls to get_parent() on various AST nodes. This is making code generation excruciatingly slow.

The simplest and fastest approach could be to simply add a "parent" attribute to each AST node to create a "doubly linked tree". However, this means that this attribute needs to be passed around in each constructor and the tree should be kept consistent. Could we implement this instead by a new "parent aware" visitor pattern? But would we then assign the parent information to each AST node instance, or to a centralised data structure inside the visitor? And would the get_parent() methods go away on all the AST node classes?

If we use the "parent aware" visitor pattern, all visitors would derive from this ParentAwareVisitor. Would it be the case that for each of their methods (visit_statement(), visit_equation(), etc.), they would need to invoke the corresponding visit_...() method in the ParentAwareVisitor? This means that we would have to explicitly make this call for all derived visitors, e.g.

class MyVisitor(ParentAwareVisitor):
    def visit_statement(self, node):
        super().visit_statement(node)   # <-- has to be done explictly to get ParentAwareVisitor to update the stack?!
        # ... things here specific to MyVisitor; can now call node.get_parent() here ...

See paragraph 8.2.3 ("Parent Aware Visitor") in the MontiCore reference manual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant