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

feat: c wrapper #2

Merged
merged 26 commits into from
Mar 7, 2024
Merged

feat: c wrapper #2

merged 26 commits into from
Mar 7, 2024

Conversation

chaitanyaprem
Copy link
Collaborator

@chaitanyaprem chaitanyaprem commented Feb 19, 2024

As integrating a c++ library to nim has some drawbacks, approach taken is to write a C API wrapper to be integrated into nwaku.

To that end, all required functionalities have been wrapped into C API.

Tasks:

  • Write C API considering BTreeMem as storage
  • Write a example to test C API
  • Improve API and resolve issues observed during integration with nwaku

Following shall be taken up separately as this PR is becoming huge:

  • Exception handling and reporting errors to nim layer
  • Optimization using callbacks in order to avoid dynamic memory allocation which would be taken up as separate PR.
  • Using LMDB storage .

Copy link

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Is there no concept of blobs in C? I see char* those are null terminated strings right?

I'm guessing blobs would be implemented the same way?

@chaitanyaprem
Copy link
Collaborator Author

chaitanyaprem commented Feb 20, 2024

Is there no concept of blobs in C? I see char* those are null terminated strings right?

I'm guessing blobs would be implemented the same way?

Actually char* is nothing but a representation of memory. Null terminated chars cause trouble only when you start using operations like length or copying a char*. since underlying implementation is c++ which uses std::string which inturn supports null chars in string, this should not be a problem.

But to ensure this doesn't cause an issue, i will add a test to validate this.

And yes, in C blobs are generally passed around as char* along with a len param.

@chaitanyaprem chaitanyaprem marked this pull request as ready for review March 6, 2024 06:34
Copy link

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Thanks!

I don't understand much of the c or cpp code but it seams we have all the functionalities we need except error handling.

Copy link

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

Just adding some comments for future review


result->have_ids_len = haveIds.size();
result->need_ids_len = needIds.size();
result->have_ids = (buffer*)malloc(result->have_ids_len*sizeof(buffer));

Choose a reason for hiding this comment

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

Where is this memory being released?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had implemented a free_result function which releases memory for all these buffers.

}

//Note: This function assumes that all relevant heap memory is alloced and just tries to free
void free_result(result* r){

Choose a reason for hiding this comment

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

ah I see, yes I think it is necessary to explore the callback approach that we have mentioned. This might bring mem leaks in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, this is a stop-gap approach as callbacks are having additional trouble in nim.
I could not use a closure based callback and hence took this approach to begin with.
I will test a callback model in sample code and then migrate to callback approach(which will not involve such memory leak issues). Also in callback i could use stack memory for many things and reduce amount of heap allocations.

cpp/negentropy_wrapper.c Show resolved Hide resolved
cpp/negentropy_wrapper.c Show resolved Hide resolved
@chaitanyaprem
Copy link
Collaborator Author

Merging this as will take up pending items in a separate PR.

@chaitanyaprem chaitanyaprem merged commit 0fb8dfe into master Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants