Implement ProgramNode#16106
Conversation
|
One or more of the following people are relevant to this code:
|
Coverage Report for CI Build 25382081104Coverage decreased (-0.1%) to 87.501%Details
Uncovered Changes
Coverage Regressions6 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
cb4cdba to
bda047a
Compare
| static EMPTY: OnceLock<DataTree<TensorType>> = OnceLock::new(); | ||
| EMPTY.get_or_init(DataTree::new) |
There was a problem hiding this comment.
Could you move the static definition outside of the method definition and give it a comment about having a single empty data tree definition to avoid reallocation on each store. The other thing is why do we need a OnceLock here? Couldn't we use a OnceCell, unless you're thinking we're going to be calling this from a threaded context. The other thought is lazy_static is an option here too.
There was a problem hiding this comment.
Could you move the static definition outside of the method definition
I'm happy to, but I'd like to know why. Isn't it a bit more readable to put it inside the method when only one method needs it?
The other thing is why do we need a OnceLock here? Couldn't we use a OnceCell
It's because static variables are always Sync and OnceCell is explicitly not Sync. Looking into lazy_static, it looks like it's been superseded by LazyOnce and LazyCell.
I made a change in 1c2eb52, but am happy to iterate it. It's worth getting the pattern right because many other nodes will want to copy it.
There was a problem hiding this comment.
I normally move it to the top of the module just to make the scoping clear. A static is global so it can be a bit confusing to see it inline. It's not a big deal either way.
1cf3d38 to
1c2eb52
Compare
|
One or more of the following people are relevant to this code:
|
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
1c2eb52 to
6276fcc
Compare
This PR closes #16029 by defining a trait called
ProgramNode. It also implements a simple core node (Store) just to show the idea of what a simple node definition will be, albeit a slightly weird one in that it requires an empty input tree.PR Stack
AI/LLM disclosure