Straightforward bugfixes and trivial additions #6105
Conversation
| _first = null; | ||
| _second = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
I actually later fixed one more bug in the VertexArray in commit
Please feel free to decide whether you'd prefer to cherrypick that from my commit and add to your change or deal with later.
Thanks a lot for doing this!
Tomas
There was a problem hiding this comment.
Thank you for incorporating the additional VertexArray fix, your change looks great to me!
|
@dotnet-bot Test this please |
| @@ -1731,20 +1752,31 @@ private Vertex ExpandBlock(int index, int depth, bool place, out bool isLeaf) | |||
| } | |||
| else | |||
| { | |||
| VertexTree tree = new VertexTree(); | |||
There was a problem hiding this comment.
Why do we need to allocate and place the VertexTree here, only to have to unplace it/update it later? Can we just do the allocation/place call in the spot where tree.Update is called now and delete the Update and Pop APIs?
There's a lot of value in having immutable data structures. Plus the fact that we need multiple callsites to Pop at each exit from the method, all guarded with an assert proves how dangerous the new API is.
There was a problem hiding this comment.
Yes, these were the two "cleanups" I originally made during my "loose rewrite" of the code from CPP. And the changes Amy is reviewing right now amount to reverting both of them and putting them back in sync with the original C++ code at
because both turned out to be incorrect.
-
The order of calling "Place" defines the ordering of the nodes within the saved vertex blob and the somewhat tricky design algorithm is apparently carefully designed so that, in each tree node, the "left" subtree is at a lower address than the "right" subtree - this is required by the internal encoding format of the runtime function table.
-
I originally removed the Pop logic because I incorrectly convinced myself that it's only used for C++ dynamic memory management (deletion of heap blocks that turned out to be unused), however in fact it turns out this is also needed for correctness of the algorithm as the recursive algorithm intentionally removes some previously added nodes.
Frankly speaking I still don't understand all details of this algorithm, I just did three iterative passes to make it behave the same as the C++ algorithm, at some point I even ended up stepping through both of them in parallel. I agree with your general observation that the algorithm could be probably made easier to reason; I'm however worried that the same holds for many other routines in CoreCLR and I sincerely doubt there's enough budget in the cross-plat AOT IL compiler project to clean them all up.
Thanks
Tomas
Added some constants to FixupConstants.cs
Removed dead code from ImportSectionsTableNode.cs
Bugfix in VertexArray