Improved traphandler code #2318
Conversation
Before it was depending on a Trait just for using a function. It makes the system on depending on a Lock when it was not really necessary. It is much simpler to depend on a function! |
| *m = handler; | ||
| /// Sets a [`TrapHandler`] for the `Store` | ||
| pub fn set_trap_handler(&mut self, handler: Option<Box<TrapHandler<'static>>>) { | ||
| self.trap_handler = Arc::new(handler); |
There was a problem hiding this comment.
So I like this, but lets make sure the new semantics are what we intended. In the old code, calling set_trap_handler would globally update the trap handler in all Stores. In the new code it only updates the trap handler for this specific store (and Stores Cloned from it). All the old trap handlers will continue to work here because they're kept alive by the Arc. (This is actually a great example of how using the raw pointer could have caused problems, the moment set is called again, the trap handler being used could have been invalid.)
If the move from Store's having 1 trap handler to Stores having N trap handlers in a tree-like structure is intentional then 👍 , if not then we probably need to rethink some of this.
And if this is intentional we may want to call it out more in the doc comments.
There was a problem hiding this comment.
Also side note but if you want to remove the extra Box in Arc<Option<Box<TrapHandler<'static>>> let me know, I think it should be possible to do so in many cases.
There was a problem hiding this comment.
I agree with the first comment :-).
| @@ -1015,7 +1018,7 @@ impl InstanceHandle { | |||
| /// Only safe to call immediately after instantiation. | |||
| pub unsafe fn finish_instantiation( | |||
| &self, | |||
| trap_handler: &dyn TrapHandler, | |||
| trap_handler: Arc<Option<Box<TrapHandler<'static>>>>, | |||
There was a problem hiding this comment.
Generally for any of these functions, but you can make them take&Arc, too, especially if they're not storing the Arc (in the cases where they just want to read it, you can even pass Option<&TrapHandler<'static>> I think by deref'ing the Arc and using as_ref on the Option)
There was a problem hiding this comment.
I agree with @MarkMcCaskey here. It avoids the need to clone the Arc, so it avoids atomic operations, and allocations.
| pub fn trap_handler(&self) -> Arc<Option<Box<TrapHandler<'static>>>> { | ||
| self.trap_handler.clone() | ||
| } |
There was a problem hiding this comment.
Idea: We can change trap_handler to return &Arc<…> rather than Arc<…>, so that we don't need clone the trap handler for every clone. It's going to be the responsability of the “user” to clone it if necessary. It will improve code even more.
| pub fn trap_handler(&self) -> Arc<Option<Box<TrapHandler<'static>>>> { | |
| self.trap_handler.clone() | |
| } | |
| pub fn trap_handler(&self) -> &Arc<Option<Box<TrapHandler<'static>>>> { | |
| &self.trap_handler | |
| } |
Hywan
added
🎉 enhancement
syrusakbary
added
the
🕵️ needs investigation
|
Closing PR, fixed by other PR #2892 |
Description
Improved traphandler code by simplifying the TrapHandler and making explicit it has to be Send + Sync.
Review