-
Notifications
You must be signed in to change notification settings - Fork 586
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
position, limit and capacity across Pointer casts #476
base: master
Are you sure you want to change the base?
Conversation
That looks OK, but the way things are right now works fine for like 99% of use cases, so unless we can figure out a way to preserve the performance without increasing the memory footprint or the workload for users not using the parser, we would need a way to disable all this alternative functionality. Could you try to work on that?
It might make it a bit faster, sure, but not as fast as no hash maps. We can modify that without changing the API though. |
Instead of trying to change everything, what about adding new overloads like |
You mean a way to create a bare pointer, with recalculated byte-wise position/limit/capacity, from any instance of Pointer ? Would that be called automatically when using a generated method working on a Pointer ? Would that break anything if the existing Pointer(Pointer) constructor did this ? |
Something like that, yeah.
No,
I keep telling you that even if we find a way to not break anything, it would also need to not add overhead! |
… `capacity` with `sizeof()` (pull #476)
I've added the scaling to the existing
Can you think of a scenario where |
I'll limit my propositions in the future for sure.
In your example if the Mat constructor used |
I'm sorry, I didn't mean it that way. I wish we could spend more time being productive, that's all, but I understand that communication is not easy, and it's not a big deal rewriting the same thing a couple of times, so please don't worry about it. I'm not angry or anything even though it may seem like that, but it's not the case. Think of it that way, a lot of people just go right into modifying the address field manually, and don't even think about anything else, so it's unlikely that users are going to end up using awkward constructs like the cast constructor or position() by mistake instead of getPointer(). I could be wrong, but since getPointer() is easier to use, I don't really see how that would happen.
But it doesn't, so everything is alright. If you find an example where this causes incorrect behavior, please let me know. |
My suggestions were typed a bit to fast I concede.
Other method could, but I guess it's rare. To work around this, if ever the user is aware that position/limit/capacity will be interpreted byte-wise for the method he wants to call, he may also use
Anyway this PR was trying to solve definitively the problem of So let's close this PR. |
I can't think of anything. The
I wouldn't consider any of this inconsistent, it's just how it works. Like I said, it's not meant to replace NIO, we have FMA for that: https://docs.oracle.com/en/java/javase/16/docs/api/jdk.incubator.foreign/jdk/incubator/foreign/package-summary.html
Can we instead try to morph this into support for FMA? |
No, I cannot.
I think you can admit that the fact that
Ok, sounds a good idea. |
👍
From Java's perspective, sure, we want to make everything the safest possible, and having slow but safe(r) get()/put() method pairs sounds good to me. Java developers are used to having methods like that, and it looks like we can make everything consistent, as long as we call something that starts with "get" like #include <stdio.h>
struct Foo {
int i = 42;
};
struct Bar : public Foo {
int j = 37;
};
void fooBar(Foo *p) {
printf("%d\n", p[1].i);
}
int main() {
Bar* p = new Bar[445];
printf("%d\n", p[1].i);
fooBar(p);
} I'm not saying we should try to make everything work exactly like C++, I'm just saying it won't be possible to make everything consistent anyway because C++ is inconsistent by design.
I'm sure there's a way to keep backward compatibility if it's just for Pointer and FMA, but for the Parser and Clang, we probably want to break backward compatibility anyway like I talked about a bit at bytedeco/javacpp-presets#475 (comment) and #453 (comment). |
BTW, if you're willing to maintain an experimental branch that breaks compatibility with JavaCPP 1.5.x, we could merge this there. Does that sound like something you'd like to do? |
Let’s be sure that there is an interest in spending resources in such project first, and that the result has a chance to be merged into JavaCPP one day. The main difference between Panama Foreign Memory API and the corresponding features in JavaCPP is the relative safety FMA provides: by default, only There is a way to create an abitrary So what do we get in breaking compatibility by adopting FMA ? I think it’s the increased safety from the user perspective. And to benefit from the efforts of the Panama team to implement optimized access to memory while keeping this safety. If we only want to support FMA API for people needing to access native memory using FMA, in order to interact with Vector API for instance, or to use the Memory Layout feature of FMA, we can simply add some method that creates a memory segment from a Pointer (or the inverse). No need indeed to break compatibility. If we do want the increased safety, then it’s a big overhaul, with the drop of Pointer classes, rewriting of Parser and Generator, and probably using a much thinner JNI layer (until the Foreign Function API supports C++ libraries). We could indeed take this opportunity to adopt Clang. But is this the direction you want JavaCPP to take ? Your priority has always been speed and giving to Java users all the options C++ developers have. It has not been safety. Is there some other aspect I missed ? |
FMA is fine, it's basically a redesigned and enhanced version of the Buffer interface. They may decide to keep it hard to use for native libraries, and I think that's what they will choose to do, so we probably won't be able to implement the equivalent of a fully functional Pointer class on top of it, but nothing is decided yet. It's probably going to take a few years before that gets settled, but in the end yeah I don't think it will be something we can use instead of Pointer. As for the FFI part of things, they (finally) made it pretty clear that the JDK will never support C++. It's just not something they will do, ever. Whatever users might want to do there, it's going to be done by higher-level libraries like JavaCPP, or some enhanced version of jextract or something. But if they keep FMA hard to use with FFI, it's never going to be easier to use than JNI anyway, so I don't see a whole lot of people adopting this. Personally, I'm waiting to see if Google decides to make Android compatible with Panama, or if they just keep using JNI, in which case everyone is probably just going to keep using JNI. That's also going to take a few years to become clear though. All that to say that I don't have a crystal ball, but I don't think OpenJDK is going to matter. Something else is going to happen at some point and my bet is on LLVM... |
This PR aims at changing the way
position
limit
andcapacity
are interpreted when thePointer
is cast from a type to another.The current behavior is counter-intuitive and may lead to difficult-to-diagnose errors for the user.
Issue
Example 1
Say you have 3D float data consisting in a series of images. You initialize the data somehow:
Then you'd like to apply some OpenCV processing on each image:
However, since this
Mat
constructor accepts aPointer
as argument, and not aFloatPointer
, its generated JNI code interprets the position (and limit and capacity) as a number of bytes and not of floats, andm
will point the the wrong slice of the buffer.To obtain the correct behavior, we must replace the code with:
Note that the call to
fp.put(imagePointer);
in the first loop doesn't need this adjustment even if it callsPointer.put(Pointer)
and not someFloatPointer.put(FloatPointer)
. This is becauseput
was hand-written and explicitly multiplies the positions bysizeof()
before callingPointer.memcpy
. If you callmemcpy
directly you'll need the adjustments.Example 2
You have a buffer of floats pointed by a
FloatPointer
of limit and capacity 100 and would like to create some view/layout on it:You would expect bp to have a limit and capacity of 400, but it actually stays at 100.
Solutions
@saudet made a change in this commit that can address Example 1, if we replace the code with:
it fixes the wrong interpretation of the position, but not of the limit and capacity. It doesn't address example 2. Example 2 could be fix with taking
sizeof
into account in thePointer
constructors though, just like inPointer.put
.This PR proposes to fix
position
,limit
andcapacity
by storing their value in byte units. So JNI generated codes will always points to the right, "physical" position. Methodsposition()
,limit()
andcapacity()
still return element-wise values, so the change should be transparent for the user.position
,limit
andcapacity
fields are made private to enforce the use of the methods in subclasses, but they could also have been kept protected and renamed aspositionBytes
,limitBytes
andcapacityBytes
.Drawbacks
Existing codes that deal with the issue by explicitly adjusting positions with the size of elements will be broken
The presets that directly access the fields should be change to use the methods
calls to
position()
,limit()
andcapacity()
are slowed down due to use ofsizeof()
that triggers a double map lookup inLoader
.Performance
To address this last point, I see three options:
Add a
sizeof
field toPointer
that could be initialized during the call toPointer.init()
. This would be the fastest but increases the size ofPointer
instances from 40 to 44 bytes for storing a value that is truly a class property.Store the
sizeof
in a single hash mapClass
=>Integer
instead of using the double hash map of offsets inLoader
.Add to the Parser the generation of an overriding of
sizeof()
(like inFloatPointer
, etc...) . Either returning a constant value if the parser can somehow obtain the value of sizeof, or something like :With the static variable initialized by the Loader.
To be verified
Adaptation of
Generator
was a bit tricky. I probably made some mistakes. There are two portions of code marked with// HG
which I'm particularly unsure about.