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

Provide a way to override memory allocator for frames #486

Merged
merged 1 commit into from
Jan 1, 2024

Conversation

EmilioPeJu
Copy link
Member

@EmilioPeJu EmilioPeJu commented Nov 30, 2022

See issue #487

@EmilioPeJu
Copy link
Member Author

Just let you know that I finished the initial implementation of the AD plugin relying on this feature, it's in https://github.com/dls-controls/ADExternal

@EmilioPeJu EmilioPeJu changed the title Provide a way to override memory management of frames Provide a way to override memory allocator for frames Sep 13, 2023
@EmilioPeJu EmilioPeJu force-pushed the switch-mem branch 2 times, most recently from 88a2ad5 to a6b6dec Compare October 5, 2023 12:25
Copy link
Member

@MarkRivers MarkRivers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the need for this capability.

I wonder if something more is needed to make this more general. For example, in a driver one might want to assign the NDArray::pData to the buffer that a vendor library allocated, to avoid the copy operation. Plugins would still use the normal allocators. The problem is that when it comes time to free() one needs a flag to indicate whether this buffer was allocated by the vendor library or by the normal mechanism. How does one do that?

@sveseli I know you have a derived NDArrayPool class. How do you handle this, and do you have any comments on this PR?

@sveseli
Copy link
Contributor

sveseli commented Oct 18, 2023

From what it looks like, the changes in this PR will work for what they were intended. However, I believe we already put in place a more general set of interfaces that would allow one to do this (the four methods starting with https://github.com/areaDetector/ADCore/blob/master/ADApp/ADSrc/NDArrayPool.cpp#L56). In the APS DAQ software we have a separate pool class for each type of DAQ object that we need to create and manage (see, for example, https://git.aps.anl.gov/C2/daq/modules/daq-base/-/blob/master/src/pva/daq/pva/PvaNDArrayPool.h#L55). This allows us to use AD framework with our own objects that inherit from NDArray.

@EmilioPeJu
Copy link
Member Author

EmilioPeJu commented Oct 19, 2023

The thing is ... making a subclass of NDArrayPool would only affect the the plugin instantiating it, and the usual case is not even that, because NDPluginDriver::driverCallback sets this->pNDArrayPool = pArray->pNDArrayPool, which means it uses the NDArrayPool of the original source of the frame.

For my application, I needed to force the source of the frames to use shared memory, the way I proposed will force every plugin and driver to use shared memory and that way my plugin can get frames from anywhere.

@sveseli , unfortunately, I don't have access permission for your link, I assume the buffers allocated by your NDArrayPool subclass were also coming from malloc which limits the applications, if not, how did you workaround the free call inside NDArrayPool::alloc?

@MarkRivers , this is a different problem right? in your case, you want to use special allocation only for the output frame of one plugin, not for everything. It's also an interesting problem, so let's brainstorm a few solutions:

  • we make NDArrayPool::alloc and NDArrayPool::release be virtual functions, however, the subclass that overrides one, should override the other one too, or they would be inconsistent. I suspect this approach is not desirable as we might want to force caching of free frames (in freeList) for a faster allocation.
  • we could override NDArrayPool::createArray returning a frame with populated pData, which is similar to passing pData to NDArrayPool::alloc but it kind of assume that it was allocated using malloc and at some point it will free it using free.
  • hook functions like NDArrayPool::onAllocateArray don't seem to help for this, as they get called too late
  • we could have a similar approach to what this PR does, we could have local malloc/free-like function pointers inside the class which could be set.
  • we could embed the malloc/free-like function pointers inside the NDArray and make NDArrayPool::alloc use those (when they are not NULL), instead of malloc/free... the pointers would possibly be set inside an overridden NDArrayPool::createArray
  • This is more intrusive, but we could allow passing the malloc/free-like function pointers as arguments to NDArrayPool::alloc, this is not desirable because we would need to change every place where it gets called.

@sveseli
Copy link
Contributor

sveseli commented Oct 19, 2023

@EmilioPeJu , if you'd like, we could probably get you read access to APS DAQ repositories, just send me email.

In our case, we do not really use internal NDArray buffer, but instead create our own objects that are destroyed when the delete is called. The IOC driver instantiates the appropriate pool class, not individual plugins (see, for example https://git.aps.anl.gov/C2/daq/iocs/daq-ps-ioc/-/blob/master/src/driver/PsDaqDriver.cpp?ref_type=heads#L339).

Looking at the NDArrayPool code again, I think you are right, there should be additional hooks that that would hide malloc/free calls.

@EmilioPeJu
Copy link
Member Author

EmilioPeJu commented Oct 19, 2023

OK, I've amended my commit and provided a hook to locally override the frame buffer allocation and deallocation functions.
The functions that you would override in a subclass are frameMalloc and frameFree.
I still keep the function to override the default malloc and free functions (which I need) but now I've made the setter function a static method of NDArrayPool called setDefaultFrameMemoryFunctions. I've also renamed a few things to be consistent with the rest and I hope the code looks cleaner now.

It can be done local to a `NDArrayPool` by overriding `frameMalloc` and
`frameFree`.
It can also be also done globally by using the static method
`setDefaultFrameMemoryFunctions`
@MarkRivers MarkRivers merged commit 7210484 into areaDetector:master Jan 1, 2024
3 of 7 checks passed
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.

3 participants