search

OP-DSL / OP2-Common

OP2: open-source framework for the execution of unstructured grid applications on clusters of GPUs or multi-core CPUs
https://op-dsl.github.io
Other
99 stars 47 forks source link

Update/docs #226

Closed gihanmudalige closed 2 years ago

gihanmudalige commented 2 years ago

Updates the readthedocs documentation and also add a tutorial. Need further text especially to document the compile-time and runtime flags.

bozbez commented 2 years ago

What behaviour do we want for the management of the user dat pointers? Currently in master it looks like the user must free their own buffers and OP2 mallocs (and then leaks for non-mpi variants? unless I've misread the code) it's own buffers. The NODEALLOC changes here change this so that the user still has to free their buffers but only after execution has terminated (API breaking - the cause of the CI failures).

I think the sensible two options here that make sense in terms of ownership are either that we keep the behaviour in master where the buffer is copied in op_decl_dat and allow the user to do whatever they want with their buffer and wherever they got it from, or for OP2 to specifically take ownership of the user-passed buffer and free it at the end of execution.

The second option would be obviously better for memory usage but breaks API and is slightly less clear for the user given the nature of C raw pointers. An improvement to that might be to use C++ move semantics with something like a std::vector to avoid the copy and prevent the user from trying anything with the data, but we would then need to extract the vector's data buffer and prevent the destructor which is very hacky.

reguly commented 2 years ago

I think the second option where user passes data to OP2, and then OP2 deallocates it is probably quite error-prone, and will lead to double frees. The cost of this initial copy in terms of time is negligible. In terms of memory usage it may be a bit more problematic, but I think it's still better to document that the user has to free it, and can do so right after op_decl_dat to save memory. We do have use case for the using the user's data pointer (with NODEALLOC), but I think this can be a "developer/ninja" feature - as it's mainly used when porting an existing code and mixing OP2 and non-OP2 code. At that point the user really has to know what they are doing. But even then, I wouldn't have OP2 free that memory.

gihanmudalige commented 2 years ago

Please double check how we handle this case in Hydra. From what I remember I had the Hydra user level code do a call op_decl_dat(), then do a hyd_free (i.e. free the user allocated memory) and then do a op_reset_data_ptr() which will reset the pointer we internally keep hold of to the new memory address we allocated internally within OP2. So this means from a user point of view (1) use their own memory, (2) read in to this block of memory (3) pass the block to OP2 when doing OP2 decl_map and decl_dat then (4) deallocate user memory (immediately after) so that we do not keep hold of a double allocation.

The "nodealloc" I only needed for the developer versions. If what I have written in the tutorial is not clear please let me know. I will revisit this again. Also I have to do a bit more to the docs before its ready.

gihanmudalige commented 2 years ago

@reguly and @bozbez, I have updated the no reallocation fix we discussed some time ago. Please do check and let me know if we are ok to merge this branch. The tutorial is now complete, but of course will be updated as we merge the new code generator.