Snapchat / djinni

A tool for generating cross-language type declarations and interface bindings. Djinni's new home is in the Snapchat org.
Apache License 2.0
166 stars 44 forks source link

Adding the option for customs constexpr string constants #157

Open jferreyra-sc opened 6 months ago

jferreyra-sc commented 6 months ago

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

LiFengSC commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

LiFengSC commented 6 months ago

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 commented 6 months ago

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?