-
Notifications
You must be signed in to change notification settings - Fork 1
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
Index fix #13
Index fix #13
Conversation
So we've figured out that this is caused by implicit data copying when the
|
There's no good way to resolve this problem. Making the |
I agree. There's no good way I can see to fix this without changing the signature of ChildenList to be a slice of pointers. That's unfortunate because it's an exported field, which means changing it is a breaking change. Nonetheless, it seems necessary. We can hope that it's mostly a no-op change, at the syntactic level, to the downstream consumers? I think often it might be. A slight blessing if so. (Considered and discarded: if only the map that's indexing children by name was constructed entirely after the children list -- then it wouldn't be grabbing pointers into a moving target, and there would be no trouble. Alas, the map is being referenced during that construction as well, so there's no easy disentangle.) |
I still have a slightly hard time estimating if the error this is fixing impacted many consumers. As an anecdata point, I just bumped a selected downstream project to use this, and it had 133 tests before, and it still has 133 tests after this bump. So, apparently, the way in which that project ranged over testmark data meant it wasn't really phased. (Or maybe it just hit a magic number that didn't hit the resizing path that encounters the bug. Seems unlikely though, considering it has a number of testmark files containing varying hunk counts and hunk name arrangements.) Nonetheless, I still agree there's a bug here and it needs fixing, and so I consider this diff on-track to merge. |
@warpfork There are now links to known downstream changes. |
I'm going to tag a v0.11.0 to include this change immediately. |
The way the indexing is built, siblings can end up missing from the ChildrenList. I'm not sure exactly how the incorrect children list is created, but switching to pointers makes the problem go away.
I think it's because the parent
ChildrenList
and theChildrenList
of a child in the parent'sChildren
map are referencing different entities. However, looking at the code it appears that this isn't possible and it makes my head hurt a little.Below is how the new test shows the failure.
Here's an absurdly large chunk of json that shows the structure of the failing bits.
$ jq '{Name: .Name, Children: .Children.one, ChildrenList: .ChildrenList[] | select(.Name == "one")}' log.json