Skip to content

Conversation

alexcrichton
Copy link
Collaborator

This commit is at attempt of mine to simplify the moving parts in how handles are specified by refactoring the related Python code which specifies how everything works. Previously I found the interactions between the various methods difficult to understand due to some layers of abstraction and where various methods were invoked. Here I've attempted to (subjectively at least) simplify a few things:

  • The add and remove methods on HandleTable no longer do any bookkeeping of borrow-related information. This makes HandleTable a pure "slab class" where it's ideally easy to see "oh hey it's a slab" and then ignore the bulk of the Python code which otherwise just says what a slab is.

  • The add_borrow_to_table and remove_borrow_from_table methods were removed in favor of "just" frobbing the borrow_count field directly. The method abstraction at least seemed to me to imply more management behind the scenes but it ended up only ever being an increment/decrement.

  • The lift_borrow_from method was renamed to track_owning_lend to indicate that it's not doing any lifting operations and it's part of the metadata tracking.

  • The metadata bookkeeping in add and remove is now inlined directly into the {lift,lower}_{own,borrow} and canon_resource_drop functions, along with the add_borrow_to_table and remove_borrow_from_table methods.

Overall to me at least this felt easier to understand where there are seven basic primitives here: resource.{new,rep,drop} plus {lift,lower}_{own,borrow} and all seven now relatively clearly document, just by looking at their single method, what each primitive is responsible for. Previously methods had to be followed to understand how the borrowing/own dynamic tracking all interacted but it's, at least in theory, more up-front now.

This commit is at attempt of mine to simplify the moving parts in how
handles are specified by refactoring the related Python code which
specifies how everything works. Previously I found the interactions
between the various methods difficult to understand due to some layers
of abstraction and where various methods were invoked. Here I've
attempted to (subjectively at least) simplify a few things:

* The `add` and `remove` methods on `HandleTable` no longer do any
  bookkeeping of borrow-related information. This makes `HandleTable` a
  pure "slab class" where it's ideally easy to see "oh hey it's a slab"
  and then ignore the bulk of the Python code which otherwise just says
  what a slab is.

* The `add_borrow_to_table` and `remove_borrow_from_table` methods were
  removed in favor of "just" frobbing the `borrow_count` field directly.
  The method abstraction at least seemed to me to imply more management
  behind the scenes but it ended up only ever being an
  increment/decrement.

* The `lift_borrow_from` method was renamed to `track_owning_lend` to
  indicate that it's not doing any lifting operations and it's part of
  the metadata tracking.

* The metadata bookkeeping in `add` and `remove` is now inlined directly
  into the `{lift,lower}_{own,borrow}` and `canon_resource_drop`
  functions, along with the `add_borrow_to_table` and
  `remove_borrow_from_table` methods.

Overall to me at least this felt easier to understand where there are
seven basic primitives here: `resource.{new,rep,drop}` plus
`{lift,lower}_{own,borrow}` and all seven now relatively clearly
document, just by looking at their single method, what each primitive is
responsible for. Previously methods had to be followed to understand how
the borrowing/own dynamic tracking all interacted but it's, at least in
theory, more up-front now.
@alexcrichton
Copy link
Collaborator Author

Oh I forgot to mention this above as well but my intention here is to preserve the exact same runtime semantics as before, only structure the code differently. I think I've managed to maintain that here but if it looks like I changed something semantically that's a mistake.

@lukewagner
Copy link
Member

Works for me, thanks.

@lukewagner lukewagner merged commit 0b5888f into WebAssembly:main Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants