Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better documentation of the context providers #48

Closed
busstoptaktik opened this issue Jul 9, 2023 · 4 comments
Closed

Better documentation of the context providers #48

busstoptaktik opened this issue Jul 9, 2023 · 4 comments
Assignees
Milestone

Comments

@busstoptaktik
Copy link
Owner

Minimal and Plain both need documentation. Perhaps taking off from some documentation of the Context trait itself.

Note that the Maximal example in the tests-folder comprises a fully self contained demonstration of how to implement a user defined context provider.

@busstoptaktik
Copy link
Owner Author

Also, the plain context could handle grids on behalf of the operations: Since we cannot destroy an operation, in any other way than dropping the context itself, there is no ownership problems here

@busstoptaktik
Copy link
Owner Author

I think the grid-handling in Plain needs some further explanation (not necessarily in a Rumination - a comment in the code would suffice): To me it looks like we should store either the raw grid-as-bytes, or (probably preferably) the final dyn Grid as Arcs in a Hashmap on first request, and then hand out Arc::clone()s to any Op requesting it.

If I understand correctly, we just make a new copy, and then needlessly Arc::clone() it every time a request is made.

Ideally we would want the Arc to self-destruct when the last Op holding a ref to it is destroyed. But as we as-of A.D. do not have an Context::op_destroy() method, this first happens once the Context goes out of scope. Nevertheless we might as well keep a list of Arc<Grid>s and then hand off Arc::clone()s to any Op asking for it?

Do you agree with this @Rennzie, or have I gravely misunderstood something?

@busstoptaktik busstoptaktik self-assigned this Nov 14, 2023
@busstoptaktik busstoptaktik added this to the 0.11.0 milestone Nov 14, 2023
@Rennzie
Copy link
Contributor

Rennzie commented Nov 15, 2023

This is probably me not fully understanding Rust, but I thought what the code was doing is what you describe here:

Nevertheless we might as well keep a list of Arcs and then hand off Arc::clone()s to any Op asking for it?

Where when the operators gridshift or deformation are constructed they requests a grid from the context and get back Arc::new(grid) (for each available grid). These references are then stored in ParsedParams.grid which is Vec<Arc<Grid>>. This means when the operator is used the new or inv functions get the vector of grids and go on their merry way.

IIUC Plain moves the grid pointer to ParsedParams during operator construction. In geodesy-wasm it works a little different. Because there isn't way a of loading a grid after the context is constructed (ie no access to the file system or until I build a way of making an http request) it must be loaded when it's created. Here I've done as you've suggested i.e store the grids in a HashMap on the conext and hand out Arc::clones() when WasmContext.get_grid() is called on operator construction.

@busstoptaktik
Copy link
Owner Author

Where when the operators gridshift or deformation are constructed they requests a grid from the context and get back Arc::new(grid) (for each available grid).

Yes, but this Arc::new(grid) is provided by the Context for the sole use of this Op, so it does not need to be reference counted at all, since every Op get their own copy.

These references are then stored in ParsedParams.grid which is Vec<Arc>.

Which is all as expected - but the Arc<Grid> should just be a light-weight clone (i.e. an Arc) of an Arc<Grid> handled by the Context, such that the same backing heap-store can be used by more than one Op. Apparently exactly as you have done with the WasmContext 🙂

So for now, I will just carry on and implement the WasmContext functionality for Plain - perhaps the two will actually end up being identical, once I learn to handle Wasm as well 🙂

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

No branches or pull requests

2 participants