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

Add mapping for std::chrono #766

Merged
merged 11 commits into from
Aug 4, 2024
Merged

Add mapping for std::chrono #766

merged 11 commits into from
Aug 4, 2024

Conversation

HGuillemet
Copy link
Contributor

@HGuillemet HGuillemet commented Jul 2, 2024

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:

  • only map the necessary functions. To add a duration in seconds s and a duration in hours h, users will have to do:
  Seconds s2 = new Seconds(s.count() + h.count() * 3600);

or:

  Seconds s2 = new Seconds(s.count() + new Seconds(h).count());
  • map all unary operators/functions, and binary operators/functions but for same types. Users will be able to do:
  Seconds s2 = s.add(new Seconds(h));

Or rather:

  Seconds s2 = Duration.add(s, new Seconds(h));

This needs about 7 Java methods per templated function/operator.

  • map all combinations, so that users can do:
  Seconds s2 = s.add(h);

or:

  Seconds s2 = Duration.add(s, h);

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

@HGuillemet HGuillemet marked this pull request as draft July 2, 2024 13:12
@HGuillemet
Copy link
Contributor Author

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

@HGuillemet
Copy link
Contributor Author

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

@saudet
Copy link
Member

saudet commented Jul 6, 2024

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
Copy link
Member

saudet commented Aug 1, 2024

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
Copy link
Contributor Author

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

pom.xml Show resolved Hide resolved
@saudet
Copy link
Member

saudet commented Aug 2, 2024

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
Copy link
Contributor Author

HGuillemet commented Aug 2, 2024

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
Copy link
Member

saudet commented Aug 2, 2024

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
Copy link
Contributor Author

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

@HGuillemet
Copy link
Contributor Author

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
Copy link
Member

saudet commented Aug 2, 2024

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

@HGuillemet
Copy link
Contributor Author

With info maps added in last commit, yes.

@saudet
Copy link
Member

saudet commented Aug 3, 2024

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
Copy link
Contributor Author

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
Copy link
Member

saudet commented Aug 3, 2024

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

@saudet
Copy link
Member

saudet commented Aug 4, 2024

So, this is only available since C++11, so do we want to make JavaCPP depend on C++11 🤔 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 #753

@HGuillemet
Copy link
Contributor Author

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

@saudet
Copy link
Member

saudet commented Aug 4, 2024

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.

@saudet saudet merged commit 2fd35c6 into bytedeco:master Aug 4, 2024
11 checks passed
@HGuillemet HGuillemet deleted the chrono branch August 4, 2024 15:10
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