-
Notifications
You must be signed in to change notification settings - Fork 39
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
Experiement: Data Oriented Design AST Sketch #144
base: devel
Are you sure you want to change the base?
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.
Not a real review, just had a couple questions while looking through the code
compiler/ast.nim
Outdated
astData: AstTree ## actual tree structure for the various AST | ||
|
||
# sparse data, not all nodes have these | ||
nodeSym: OrderedTableRef[NodeId, PSym] ## symbols |
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.
Why use table ref in addition to the state ref? We would be doing triple indirection in order to get the necessary data - (1) unref state, (2) unref table, (3) go to the table data store.
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.
Was bashing at something that wasn't working at the time, didn't revert.
compiler/ast.nim
Outdated
|
||
type Gconfig = object | ||
# we put comments in a side channel to avoid increasing `sizeof(TNode)`, which | ||
# reduces memory usage given that `PNode` is the most allocated type by far. | ||
comments: Table[int, string] # nodeId => comment | ||
comments: Table[NodeId, string] # nodeId => comment |
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.
Pretty sure you have already though about this, but we can put comments in the AST state as well, and make all comment accessor functions side-effect free as well
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.
I did, just wanted to get things working first, but the VM is being a jerk.
9f7c768
to
e554bd2
Compare
Breaks/Issues: - no more nil means have to use a sentinel value - lots of effects because of access to `ast_types.state` - tcan_compile_nim, tcompilerapi, and tresults fail - the ic test category takes forever/doesn't come back Next steps: - test fixes - it's pretty slow right now - clean-up type and report storage, tables are super slow signature change for transition procs mean we're creating new node this could well break assumptions that pervade the compiler. Worst case will need to supprot the mutability for now to get the first port, then figure out how to start changing up the compiler based on fork/join/mutation observations.
resurrected, this was far less painful, it's getting very close to allow for some discussion, just a few clean-up items but that can be somewhat concurrent. Immediate Items:
Big thanks to @zerbina their VM rework has been a huge unblocker. |
First pass attempt at drafting a DOD AST based on this discussion:
#139
Breaks/Issues:
ast.transitionX
procs now return a new nodesignature change for transition procs mean we're creating new node
this could well break assumptions that pervade the compiler. Worst case
will need to supprot the mutability for now to get the first port, then
figure out how to start changing up the compiler based on
fork/join/mutation observations.
Blocker(s):
The VM takes address of nodes, but now transitions might mean those are
no longer valid. Even if we rely on id as address, this is likely not
the right semantics, as we want to point to the AST "position", for
example when we're doing constant expression evaluation incrementally
Reviewers: