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

WIP: Use sun.misc.Unsafe in pointers and not in indexers #408

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matteodg
Copy link
Member

sun.misc.Unsafe is used in raw indexer, but it should be really used in Pointers: the indexers are just a layer on top of array/buffer/pointers.

Still unfinished as it is not clear how to handle bool, clong, size_t pointers.

@saudet
Copy link
Member

saudet commented Jun 22, 2020

This is merely duplicating what we already have in Indexer. If this is all the functionality you need, please use Indexer instead.

@matteodg
Copy link
Member Author

I'm using Indexer as you suggested, but maybe I'm missing something: isn't it the Indexers just a layer on top of array/buffer/pointer? Why are they accessing internal pointer structures using Unsafe, shouldn't the Indexers just call pointer.get()/pointer.put()?

@matteodg
Copy link
Member Author

Another example: I'm using the HDF5 JavaCPP preset and it takes and returns pointers objects, why I should wrap the pointer in an Indexer to access its data? I would like just to consider the pointer as a basic data storage (just like array or buffers), not a Indexer with a shape and indexing.

@saudet
Copy link
Member

saudet commented Jun 22, 2020

Yes, Indexer is basically a wrapper for arrays, buffers, and pointers, and we need to use sun.misc.Unsafe for arrays and buffers as well, it's not just a pointer thing...

@matteodg
Copy link
Member Author

I cannot find where Unsafe is used to access arrays or buffers: I'm looking at FloatArrayIndexerand FloatBufferIndexer for example, I guess I'm missing something obvious here

@saudet
Copy link
Member

saudet commented Jun 22, 2020

Another example: I'm using the HDF5 JavaCPP preset and it takes and returns pointers objects, why I should wrap the pointer in an Indexer to access its data? I would like just to consider the pointer as a basic data storage (just like array or buffers), not a Indexer with a shape and indexing.

That's what Pointer does, using JNI. How would you use sun.misc.Unsafe to access something like std::map<std::string,std::string>?

If you can do everything you need with Indexer though, what is the issue?

I cannot find where Unsafe is used to access arrays or buffers: I'm looking at FloatArrayIndexerand FloatBufferIndexer for example, I guess I'm missing something obvious here

https://github.com/bytedeco/javacpp/blob/master/src/main/java/org/bytedeco/javacpp/indexer/ByteArrayIndexer.java and we would also need it for Buffer if/when we decide to add optimizations to skip bounds checking, see issue #156.

@matteodg matteodg force-pushed the use-unsafe-in-pointers-not-in-indexers branch from 4abc7de to 0d92845 Compare June 22, 2020 16:06
@matteodg
Copy link
Member Author

OK, I see: for primitive pointers (whose element is a primitive in Java) we can actually use Unsafe for get() and put(), but for more complicated pointers (for any struct for example std::map<std::string,std::string>) I guess you need JNI because they might be aligned differently in different architectures? Did I understand correctly this is your point?

@saudet
Copy link
Member

saudet commented Jun 23, 2020

Right, the definition of classes like std::map<std::string,std::string> is implementation dependent, so we basically need to run C++ code to access them.

If you're just worried about creating an instance of Indexer, we could add those as static methods like FloatIndexer.get(FloatPointer p, int i), which would be a lot easier to maintain and I think would also make more sense.

@matteodg
Copy link
Member Author

I was actually more concerned from an architecture point of view: I see now Pointers in general have been created for the JavaCPP bridge purposes, so to account for structs/classes/unions in C++.
But in hindsight I was considering primitive Pointers being like a drop-in replacement of Java Buffers, so I expected them to be as fast as possible. What about creating a PrimitivePointer abstract class as a super class for all primitive pointers and have them use Unsafe for get()/put()?

And have Indexers just a layer on top of arrays/Buffers/Pointers/MemoryHandles/etc. for shaping and indexing purposes.
In fact Indexers are not really used to deal with structs/classes/unions from C++.

@saudet
Copy link
Member

saudet commented Jun 24, 2020

I'm repeating myself, but we need sun.misc.Unsafe for buffers and arrays as well. What you're proposing makes it harder to maintain. You could fork JavaCPP and maintain that if you want, but someone will need to maintain it and that person is not me.

@saudet
Copy link
Member

saudet commented Jun 24, 2020

Also, don't forget to consider what to do when sun.misc.Unsafe is not available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants