Stream: git-wasmtime

Topic: wasmtime / issue #6421 Add `EntityList::copy_from`


view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 19:44):

jameysharp commented on issue #6421:

I'll leave detailed review to @elliottt but I took a look and I'm having a difficult time convincing myself that this implementation is correct. Some things I'm unclear on:

Also, I'd love to hear more about what motivated adding this.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 20:00):

PROMETHIA-27 commented on issue #6421:

This ended up getting refactored out for now, but the original need was that I had an iterator of EntityList and wanted to produce a new EntityList from their concatenated values (without having an intermediate allocation). This caused borrowck issues with .extend and from_iter because I can't borrow the slice of values and mutate at the same time.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 22:55):

jameysharp commented on issue #6421:

Yes, other shouldn't move, but self may. What I missed was that normally self.index is only read after grow_at returns, so it reflects the result of any reallocation.

However that's not actually true if other is None. If self gets reallocated _and_ we're moving within the same list, then the source range will point into the list which was just freed. That still gets the correct result in the current implementation since the old list's contents don't get overwritten immediately, but it's questionable.

I have a similar concern about moving within the same list with regard to the documentation for grow_at, which claims the newly-inserted elements have no specified value. So if the source overlaps with the destination, where the destination was allocated with grow_at, then some portion of the source is copied from unspecified values. Again I _think_ this gets the correct result in the current implementation, because grow_at actually leaves behind the old values in those elements, but it's hard for me to convince myself that it's right and that future changes elsewhere won't break it.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:34):

PROMETHIA-27 commented on issue #6421:

That's a very good point, the reallocation when moving within self demonstrably breaks- I ended up with an Inst0 in a vec that had only Inst1-4, on top of otherwise being incorrect.

I can add extra logic to handle this case, or just ban the "copy within one list" case. The logic would be somewhat verbose and I'm not sure if anyone will ever need that functionality, but I might do it just for completeness' sake.

For now I'm going to push the "ban" option along with some extra tests and if it's agreed that the extra complexity isn't worth supporting the other case then I'm fine with it being merged in that state, I certainly don't need that functionality.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2023 at 23:39):

PROMETHIA-27 edited a comment on issue #6421:

That's a very good point, the reallocation when moving within self demonstrably breaks- I ended up with an Inst0 in a vec that had only Inst1-4, on top of otherwise being incorrect.

I can add extra logic to handle this case, or just ban the "copy within one list" case (which I forgot implicitly happens because of borrowck without an Option). The logic would be somewhat verbose and I'm not sure if anyone will ever need that functionality, but I might do it just for completeness' sake.

For now I'm going to push the "ban" option along with some extra tests and if it's agreed that the extra complexity isn't worth supporting the other case then I'm fine with it being merged in that state, I certainly don't need that functionality.


Last updated: Dec 23 2024 at 12:05 UTC