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

Implement Python buffer protocol. #238

Closed
wants to merge 2 commits into from

Conversation

ast0815
Copy link
Contributor

@ast0815 ast0815 commented Feb 25, 2021

Hello,

I am currently working on a piece of DAQ software that uses the Python uhal bindings. Performance is an issue, so I tried to get the data transfer into the Python environment as fast as possible.

For this, I implemented the "buffer protocol" which allows Python objects to access each others underlying memory. It makes e.g. conversions of the read out ValVectors into Numpy arrays much much faster.

I tried converting the ValVector directly arr = np.array(valvector) or via the value method arr = np.array(valvector.value()).

Without my patch:

Reading 1000 blocks with 32768 words each...
Time: 1.855031
With array conversion...
Time: 197.910151
With array value conversion...
Time: 7.071396

With my patch:

Reading 1000 blocks with 32768 words each...
Time: 1.896150
With array conversion...
Time: 1.882111
With array value conversion...
Time: 6.806115

As you can see, when the buffer protocol is implemented, a direct conversion to an array happens pretty much instantaneously, since the memory does not even have to be copied.

Please let me know what you think!

@tswilliams
Copy link
Collaborator

Hi,

Thanks for these changes. They look good to me, with just one caveat: As is done in the other ValVector data access methods, can you update the data method to check the value of mMembers->valid (and throw if that's false). I'll put a comment to that effect in the code review now.

Cheers,
Tom

template< typename T >
T* ValVector< T >::data() const
{
return mMembers->value.data();
Copy link
Collaborator

@tswilliams tswilliams Mar 6, 2021

Choose a reason for hiding this comment

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

Please change this implementation to:

    if ( mMembers->valid )
    {
      return mMembers->value.data();
    }
    else
    {
      exception::NonValidatedMemory lExc;
      log ( lExc , "Access attempted on non-validated memory" );
      throw lExc;
    }

Return the address of the underlying memory
@return the address of the underlying memory
*/
T* data() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change the return type to const T* (to match the return type constness of the std::vector data() const method)

@ast0815
Copy link
Contributor Author

ast0815 commented Mar 8, 2021

I implemented the requested changes, but I cannot test them right now. Unfortunately the hardware we are using this with died last Friday. I will check it a soon as it works again, but feel free to merge anyway if you think the changes look good.

@ast0815
Copy link
Contributor Author

ast0815 commented Apr 21, 2021

Anything else keeping this from being included?

@tswilliams
Copy link
Collaborator

Hi @ast0815

Sorry for the delay in merging this. I rebased this branch against the master branch a couple of days ago, and added some related tests to reduce the chances of breaking this in the future. I've now merged those changes in through another pull request - #253 - and created a new tag containing these changes: v2.8.1

Cheers,
Tom

@tswilliams tswilliams closed this Jun 20, 2021
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.

2 participants