atonement for compiler abuse (type stable cursors!) #120
Conversation
Codecov Report
@@ Coverage Diff @@
## master #120 +/- ##
==========================================
+ Coverage 66.40% 70.04% +3.63%
==========================================
Files 8 8
Lines 384 434 +50
==========================================
+ Hits 255 304 +49
- Misses 129 130 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
from an initial glance, this looks good! I'll try it out this week and see if it also works well. |
ExpandingMan
changed the title
|
Alright, this should be done now. I have also added |
|
This is a decent amount slower for me than #118 (roughly 2x) in the |
|
I'll look into it. The Just to be clear, it's slower to run, slower to compile, or both? |
|
On my branch, for the first 2 runs, I'm seeing and on yours so runtime is similar, but compile time on my pr is slightly better. |
|
I really don't think we should be chasing down compile time differences like that unless there is a simple and obvious fix. Not only is it hard to optimize but it will not even be the same across different versions of Julia. As long as we can eliminate any pathological compiler behavior like what I had before I'd ask that we not worry about it. At the very least, I don't think it's a great idea to design the packages data structures around this, if someone wants to come in and optimize it away later they can. Also, for what it's worth, there's not going to be any way around the fact that this package will be more taxing on the compiler than most due to the recursive and unpredictable nature of type-unstable trees. |
|
agreed. As one last thing, can you add my definition of |
|
Will do. Also, any thoughts on version bump? I really think we can get away without tagging this as breaking. It didn't even break our own unit tests. It's unlikely that anyone is even using cursors directly let alone relying on their type signature or concrete type. At most maybe we can solicit opinions on discourse or something. |
|
I think this doesn't need a major version bump. From what I've seen, no one relies on this (and I went through most of the dependents in the bump to 0.4 to update project.tomls). |
|
@oscardssmith that I just did a quick search and I don't think there's anywhere we currently rely on I bumped the version to 0.4.3. |
|
I don't really care about the implementation, but I do think it's important to have ImplicitCursors provide forward and reverse iteration over siblings. Another way to do it would be to make the |
To do that we'd have to somehow store multiple completely distinct states because it can't be guaranteed in the general case that the state used by Also, don't forget, even if we do store a sibling index, the cursors would still have to repeatedly iterate over the parent children because it's still not guaranteed that |
|
@oscardssmith could we do |
|
yeah. that works. |
|
@oscardssmith any objections to merging this? |
|
go for it. |
|
lol I don't have permissions, I wasn't saying that I was going to do it 😅 |
|
Btw, if you have any lingering concerns about breakage now is the time to discuss them... I'm not worried because we did not even break our own unit tests, and it's not like our unit tests are that bad. Also back when I did the major PR I ran tests on the entire ecosystem and almost nothing broke and that was a much bigger change, so I think I'm willing to have someone come along and curse me out if something bad happens. |
|
I have to change a few minor things, but the changes are pretty minor and I'm using more of the API than anyone else. |
|
I'll merge this next week if no one objects. |
This PR does the following:
StableCursorhas been added to give type-stable iteration over trees withHasNodeType.StableIndexedCursorhas been added which allows really, really type-stable iteration over trees with bothHasNodeTypeandIndexedChildren. That is, unlikeStableCursor, it does not rely on any crazy inference shenanigans likeIterators.approx_iter_type. If you want really fast tree traversal, you should define a node type that can use this.I have not comprehensively benchmarked this, but so far according to
@code_warntypeand@btimeit looks like this is doing more-or-less what we want. In particular, it definitely is type stable forHasNodeType(though if you repeatedly callStableCursorit will repeatedly callIterators.approx_iter_typewhich is slow, so it's not perfect).I have a bunch more things I'd like to do in this PR, but I wanted to get it in front of maintainers since this is all very tricky and confusing. There is a fair amount of code duplication here, far more than I would permit in most circumstances. Arguably we should find a way to combine
ImplicitCursorandStableCursor. However, I find all this so subtle, confusing and tricky that I think it will be a boon rather than a burden to the ongoing maintenance of this package to allow for a bit of code duplication. If I were to really follow all the way through and combine everything as much as possible the result would be far too clever and would be much more difficult to maintain than what's here now (to me at least).TODO
AbstractNode(Add an abstract type forAbstractTreestoAbstractTrees.jl#115). Fortunately we will have 2 types in the package which can inherit from this.Union{Nothing,T}whereTis the arrayeltype. Since this is the only way to get fast tree traversal we should make it as easy as possible. This shouldn't be hard, it will merely involve creating some simple recursive constructors for a newStableNodetype.Breakage?
Technically anything we do to fix #117 will be breaking because it's impossible to fix without changing the type signatures of all
TreeCursors. However, I rate it extremely unlikely that it will actually break any packages. In fact, it shouldn't even break unit tests. Therefore it's worth discussing whether its' worth tagging 0.5. Because of the ubiquity of this package in the ecosystem it might be worth running automated tests on dependencies to see if we can manage it.