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

Adding the option for customs constexpr string constants #157

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jferreyra-sc
Copy link
Contributor

@jferreyra-sc jferreyra-sc commented Dec 16, 2023

Context

The motivation for this change is that string constants are defined in C++ as static const std::strings, which during the program startup they do allocations on the heap and copy data.

Changes

This PR adds a couple cli options which will allow the user to specify a constexpr string constant type, for example, std::string_view and const char*.
As well it allows to optional include a header file (for example <string_view>). In the case of using const char* the optional header can be undefined and no header file will be included.

Testing

  • I added a string constant to the example IDL, and observed how the static const std::string was defined in the generated header, the <string> header file included and in the generated source the definition of the constant was added.
  • Then I added the options lines to the build script (examples/run_djinni):
    --cpp-string-constexpr "std::string_view" \
    --cpp-string-constexpr-header "<string_view>" \
  • Build again, observed how the string constant is only defined in the header with the proper include file.
  • Changed it for const char* and removed the header option from the build script, observed how the string constant was defined with the primitive type and no include files were added.
  • Built and launch both iOs and Android projects with all these configurations and observed proper functionality.

@li-feng-sc
Copy link
Contributor

The reason they are not string_view by default is because djinni string is mapped to std::string in C++. So if a function takes a string parameter, passing a string_view will result in a temporary std::string gets created. So while we saved a static constructor, we might incur the overhead of multiple constructor calls when calling functions.

If we change djinni string parameters to use std::string_view in C++, this will result in a mismatch between djinni strings in results and records vs as parameters. I personally feel that complicates things too much.

@remyjette
Copy link
Contributor

The reason they are not string_view by default is because djinni string is mapped to std::string in C++. So if a function takes a string parameter, passing a string_view will result in a temporary std::string gets created. So while we saved a static constructor, we might incur the overhead of multiple constructor calls when calling functions.

At least in my library, we would rather the small overhead of having to potentially allocate the string if we end up taking one of those constants and passing it to a Djinni function, rather than paying the startup cost of heap allocating all strings during program boot.

Given that this is an option and is by default disabled, callers who would prefer to keep the strings in static storage for the lifetime of the program can still do that. But I would really appreciate having the option to decide which approach makes most sense for me.

@jferreyra-sc
Copy link
Contributor Author

jferreyra-sc commented Dec 19, 2023

The reason they are not string_view by default is because djinni string is mapped to std::string in C++. So if a function takes a string parameter, passing a string_view will result in a temporary std::string gets created. So while we saved a static constructor, we might incur the overhead of multiple constructor calls when calling functions.

At least in my library, we would rather the small overhead of having to potentially allocate the string if we end up taking one of those constants and passing it to a Djinni function, rather than paying the startup cost of heap allocating all strings during program boot.

Given that this is an option and is by default disabled, callers who would prefer to keep the strings in static storage for the lifetime of the program can still do that. But I would really appreciate having the option to decide which approach makes most sense for me.

Actually it just make sense to make the interface functions also to take a string_view by value as parameter in C++.
Let me see if that is possible, if I can code it without making a mess.

@li-feng-sc
Copy link
Contributor

Actually it just make sense to make the interface functions also to take a string_view by value as parameter in C++. Let me see if that is possible, if I can code it without making a mess.

Performance wise you are correct. But this means we cannot simply tell the user that djinni string is std::string in C++. They are string_view when used as single parameter, and std::string as return values or record members.

@remyjette
Copy link
Contributor

But this means we cannot simply tell the user that djinni string is std::string in C++.

As long as that's opt in behavior then that's probably a reasonable tradeoff?

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.

3 participants