bytedeco / javacpp

The missing bridge between Java and native C++
Other
4.48k stars 581 forks source link

Add mapping for `std::chrono` #766

Closed HGuillemet closed 2 months ago

HGuillemet commented 3 months ago

Add support for the standard chrono library, sticking to C++11 (not the additions of C++20).

std::chrono makes heavy uses of templates. This PR add mapping for the classical integral durations, and 2 floating point durations.

Concerning the instantiation of function and operator templates (mathematical operations, comparisons...), we have 3 options:

HGuillemet commented 3 months ago

On Linux, with option 1, libjnijavacpp.so is 153K. Without this PR: 63K.

HGuillemet commented 3 months ago

A fourth option is to keep template instantiations low and add pure java helper functions using generics or inheritance to improve the API.

saudet commented 3 months ago

A fourth option is to keep template instantiations low and add pure java helper functions using generics or inheritance to improve the API.

No, let's keep this as close as possible to the C++ API.

saudet commented 2 months ago

This needs about 28 Java methods per binary templated function/operator.

We can always add those at a later point in time right? It won't change the current code in this PR?

HGuillemet commented 2 months ago

It will just add more java mappings. And inflate the size of libjnijavacpp.so which is already more than doubled with option 1.

saudet commented 2 months ago

Please add a dummy src/main/java/org/bytedeco/javacpp/presets/chrono.java with @Properties(target=... to that package so that users can do @Platform(inherit=... on it.

HGuillemet commented 2 months ago

Done, but: 1) I had to add an empty Chrono class to act as a the global class. In this case we have a target directory but no global class, but it's not supported. Maybe you need a special value for the global property to mean no global. The empty string ?

2) I expected that the <argument>org.bytedeco.javacpp.chrono.*</argument> won't be necessary anymore, but it is. I haven't investigated why.

3) With the presets class, there must also be a way to put the JNI code in a separate library jnichrono. Do you want that ?

saudet commented 2 months ago

Right, we probably need to add a dummy global.chrono.class that extends from presets.chrono.class and make the other classes inherit from that.

I guess we could do a jnichrono, but let's get the basics working first

HGuillemet commented 2 months ago

That's committed, and seems to work, but I didn't make the dummy global class extending the presets class

HGuillemet commented 2 months ago

I added the property inheritance of chrono classes towards chrono presets, and chrono presets towards javacpp presets. It seems more logical but doesn't change anything.

saudet commented 2 months ago

So, does it work like that with @Platform(inherit=... for PyTorch?

HGuillemet commented 2 months ago

With info maps added in last commit, yes.

saudet commented 2 months ago

That's a bit weird, I don't know where that would be needed. Anyway, I guess that's OK as a workaround.

What happens if we do global = "org.bytedeco.javacpp.presets.chrono" though, does everything still work?

HGuillemet commented 2 months ago

That's a bit weird, I don't know where that would be needed. Anyway, I guess that's OK as a workaround.

When the headers of a client presets like pytorch are parsed, if a std::chrono::duration<long,std::ratio<1,1000>> is found, how would it be mapped to org.bytedeco.javacpp.chrono.Milliseconds without such infomap ?

What happens if we do global = "org.bytedeco.javacpp.presets.chrono" though, does everything still work?

Not tested but should work. Class inheriting would have a static import org.bytedeco.javacpp.presets.chrono.* but since there is no static members in the presets class, that shouldn't arm. However, there are a some global functions in chrono that I didn't map yet that would fit in Chrono, like duraction_cast. This one has 2 template arguments, so would inflate the JNI if I do. Maybe keep the empty Chrono class for now if we fill it later ?

saudet commented 2 months ago

Sounds good, let's just rename the global class to global.chrono though

saudet commented 2 months ago

So, this is only available since C++11, so do we want to make JavaCPP depend on C++11 :thinking: I guess it's as a good time as any to do it... If that's the case, then let's also just use char16_t to fix https://github.com/bytedeco/javacpp/pull/753

HGuillemet commented 2 months ago

That or a separate presets. Also before merging, shall we try to make a separate JNI lib or it's not worth ?

saudet commented 2 months ago

Nah, let's leave more complicated stuff for another time. And let's just make C++11 the minimum requirement, feels fine to me at this point in time.