| 12:33 | <Andreu Botella> | I wonder if it makes sense to add a method to the C++ embedder API for AsyncContext.Snapshot to tell whether two objects represent the same underlying map |
| 12:40 | <Andreu Botella> | also to create an AsyncContext.Snapshot with an empty map (well, with the map being undefined in my V8 implementation) |
| 13:05 | <littledan> | Do you have use cases for those in mind? |
| 13:06 | <littledan> | For the latter, I really don’t want to expose that to JS, and I wonder why it would ever be valid to do |
| 13:17 | <Andreu Botella> | not to JS, to embedders |
| 13:31 | <Andreu Botella> | because of reasons involving the architecture of Blink, task attribution is using a std::unique_ptr<Scope> as an RAII scope for the equivalent of snapshot.run() |
| 13:33 | <Andreu Botella> | it has to be an allocation because users of this scope can't depend on the actual C++ files calling into V8, so Scope there has to be an abstract class that is implemented elsewhere |
| 13:33 | <Andreu Botella> | you can save the allocation if you can be sure that the snapshot you'd be running is the same as the current one, and if there's nothing new to set task allocation-wise that wasn't there before |
| 13:34 | <Andreu Botella> | by returning a null unique_ptr |
| 13:36 | <Andreu Botella> | the snapshot with an empty map, I just realized I don't actually need it in Blink |
| 21:01 | <littledan> | the snapshot with an empty map, I just realized I don't actually need it in Blink |
| 21:03 | <littledan> | because of reasons involving the architecture of Blink, task attribution is using a |
| 21:03 | <littledan> | you can save the allocation if you can be sure that the snapshot you'd be running is the same as the current one, and if there's nothing new to set task allocation-wise that wasn't there before |
| 21:14 | <rbuckton> | I take it you're not talking about an RAII pattern in JS, correct? I saw RAII and immediately thought of using declarations. If that's unrelated, feel free to disregard this. |
| 21:28 | <Andreu Botella> | Yeah, we were going to make the API look like that too, weren't we? std::unique_ptr. You could do the same with the embedder APIs we have by wrapping them in std::optional, but you're still using that memory. With unique_ptr you can save the allocation if not needed. |
| 21:29 | <Andreu Botella> | How could we do this RAII pattern only conditionally? Anyway couldn't this optimization be contained within the implementation of .run/the RAII equivalent? run, and it would only save on the allocation if both are unnecessary. The embedder API could also do that, if it has a combined RAII scope for snapshot and variable run. Not otherwise. |
| 21:39 | <Justin Ridgewell> | I take it you're not talking about an RAII pattern in JS, correct? I saw RAII and immediately thought of |
| 21:41 | <littledan> | I'm really confused, why not just unconditionally stack-allocate this memory? |
| 21:41 | <littledan> | it's not very much, is it? |
| 21:41 | <littledan> | it's just to remember to restore to the previous snapshot (if there was a change) |
| 21:59 | <Andreu Botella> | I'm having to explain implementation decisions about task attribution that don't make sense outside of Blink (not even V8) |
| 22:00 | <Andreu Botella> | it's because of the way Blink prevents dependencies between different parts of the code, so the task attribution scope as exposed by the task attribution API must be an abstract class, with a subclass elsewhere in the code that can depend on the relevant things |
| 22:03 | <Andreu Botella> | I'm not sure it's even worth skipping that allocation though |
| 22:03 | <littledan> | oh, the linking structure prevents you from stack-allocating it? OK |
| 22:05 | <littledan> | in a lot of cases, Blink builds somewhat complicated layers on top of V8 to make it suitable for Blink. I imagine that this would be a pretty simple layer to build on top of the snapshot class, right? Could this just live in Blink? |
| 22:05 | <Andreu Botella> | the intent was for it to live in Blink |
| 22:06 | <littledan> | ah OK. this is just about the underlying primitives needed to make that |
| 22:06 | <Andreu Botella> | yep |
| 22:06 | <littledan> | OK, that seems fine to me, to add the comparison function, if it's useful for this |
| 22:07 | <Andreu Botella> | the alternative is for v8::AsyncContext::Snapshot to represent the internal map rather than a JS object, and that way you could compare them directly with the equals operator |
| 22:09 | <littledan> | oh, that too--I don't think it needs to represent the JS object |
| 22:09 | <littledan> | we should avoid allocating a JS object when it's never going to be exposed to JS anyway |
| 22:09 | <Andreu Botella> | yeah, I suspect my refactor of task attribution on top of AsyncContext might end up leaking realms because of that |
| 22:10 | <Justin Ridgewell> | Ditto. Us repurposing CPED requires a JS Object, but with a real API we should be using C++ datastructures |