feat: zero relocation string containers #159
Conversation
| template <typename T, typename U, typename Compare, std::size_t... KNs, | ||
| std::enable_if_t<!bits::is_pair<Compare>::value>* = nullptr> | ||
| constexpr auto make_map( | ||
| Compare const& compare, |
There was a problem hiding this comment.
Note that this has the comparison functor as it's first parameter instead of the last because otherwise this overload is sometimes ambiguous with the overload without comparison functor. (Probably due to the parameter pack).
| return at_impl(*this, key); | ||
| } | ||
| constexpr Value& at(Key const &key) { | ||
| constexpr decltype(auto) at(Key const &key) { |
There was a problem hiding this comment.
decltype(auto), unlike the explicit reference, allows our use of a proxy iterator by allowing this to return on-demand constructed by-value types to be returned here (mostly string_view/frozen::string). Otherwise we'd return a reference to a temporary, which is UB.
|
|
||
| } // namespace bits | ||
|
|
||
| // Helpers to preserve arrays in type information, instead of letting them decay to pointers |
There was a problem hiding this comment.
This is essentially std::make_pair with the added twist that it avoids decaying of arrays to pointers. This function is the primary thing I'm not super happy with. But I couldn't find any other way to preserve the array size (string size) in the type information otherwise.
It cannot be put in an argument to a constexpr function either, because the values of parameters to constexpr functions must also be usable at runtime, and thus cannot become part of the type info later on (i.e. storage_size template parameter of return type of factory function make_XXX).
|
Well, I was not expecting those build failures on MSVC. I'll try to resolve those tomorrow. |
|
It appears I've managed to discover some bugs in MSVC. Hopefully my last commit successfully works around them (I may still have to add or subtract one level of I've had some discussions in the CppLang slack about this https://cpplang.slack.com/archives/C21PKDHSL/p1692113981628139 (invite, if required, here: https://cppalliance.org/slack/). It appears that MSVC is definitely buggy with some combination of parameter packs, NTTPs (non-type template parameters) and reference-to-bounded arrays. There appear to be multiple ways to work around this. I've gone for the one that seemed most palatable to me (adding and using wrapper type |
|
I finally managed to get a Windows VM with VS 2017 (because AppVeyor runs with 2017). After 5352523 I don't have any build errors locally anymore. So hopefully AppVeyor concurs... |
|
@serge-sans-paille I think you need to trigger the build on AppVeyor? Could you do that (or, if I'm wrong, tell my how to do so myself). |
|
@muggenhor I've removed appveyor and travis CI in favor of github actions, could you rebase on master branch first? |
muggenhor
force-pushed
the
feat/zero-reloc-string-containers
branch
from
5352523 to
cb357a8
Compare
Unfortunately the 'push' event doesn't trigger when a PR originates from a fork. So trigger on the 'pull_request' event too.
muggenhor
force-pushed
the
feat/zero-reloc-string-containers
branch
from
1479ccb to
aae5197
Compare
1. Instead of separate mkdir build && do stuff in build: let CMake handle this itself with its -B <build-dir-mkdir-on-demand> and -S <srcdir> options. 2. Tell GCC/Clang to color their diagnostics even though their output is captured in a pipe (!isatty(STDOUT_FILENO)).
…struction Specifically don't pin it to 'carray'. This makes it possible to change the item storage later on.
This allows using more efficient underlying storage containers for specific cases where it's possible to be more efficient than the current naive array of `value_type[N]`.
When our underlying iterator is a proxy iterator it's dereferencing operator (`*`) will return a temporary. The .at() method should *not* return a reference to it or one of its members (->second). Instead it's `second` member should be returned as is. A value when it's a value type, a reference when it's a reference type.
Specifically this version stores strings in a way that doesn't require dynamic relocations when part of position-independent-code.
These specializations will cause the string storage to be contained inside the 'set' and 'unordered_set' instances by using the new PIC-friendly container. Sets constructed this way should not add any extra dynamic relocations and their associated costs and restrictions.
This is necessary for storing strings in maps in a way that doesn't require dynamic relocations when part of position-independent-code. Using the tuple accessor (meta-) functions has the side benefit of this being generic enough to be useful (without modifications) for something like a multi_index type in the future.
These specializations will cause the string storage to be contained inside the 'set' and 'unordered_set' instances by using the new PIC-friendly container. Sets constructed this way should not add any extra dynamic relocations and their associated costs and restrictions.
For large string tables (large number of elements) with relatively short strings this has a huge effect. Easily cutting such a string table in half. For unordered maps/sets this is exclusively a size benefit. For ordered maps/sets this significantly increases the probability of L1 cache hits and thus also provides a potential performance benefit.
…rectly Specifically it fails to consider a dependent type for overload resolution even when the template parameter it's dependent on is specified explicitly. So we're using enable_if-based SFINAE instead now to check for the presence of a `value_type` member type in `T`.
…am packs It's unclear what exactly is going on here. But use of a reference to bounded-array as a template parameter inside a parameter pack cause MSVC to emite useless compiler errors. On MSVC 19.16 it appears to eliminate these overloads from consideration without any diagnostic (like SFINAE). On more recent versions it provides the supremely useless "failed to specialize function template" without informing about *why* it is failing to do so. And some variations of these, applying decltype to NTTPs, even ICEs the compiler. Converting `const T (&)[N]` to `array_ref<T, N>` allows us to *not* have a reference-to-array expression inside the parameter pack and this appears to stop triggering all compiler bugs discovered thus far.
For some unknown reason MSVC fails to evaluate at constant-evalutation-time (constexpr) pic_array::appender::operator() when it uses a range-based-for expression. Manually expanding it, in the exact same way that the C++ spec says "the implementation" (compiler) should do it, allows it to proceed.
muggenhor
force-pushed
the
feat/zero-reloc-string-containers
branch
from
aae5197 to
a7d4008
Compare
|
I had to do some changes to get CI to work:
(2) and (3) are not strictly necessary. So I can remove them easily enough if you dislike those changes. Also I used interactive rebase to make them the commits closest to |
|
I started reviewing the changes... but there are too many of them. Could you split this in individual PRs I can review independently? The time I have to spend on frozen is unfortunately limited :'( |
|
I can split it, but there's dependencies between the changes, so there are some limits to how far I can split it. Either way, I'm also somewhat restricted in time. I was hoping to do some splitting this week but didn't end up having any time. Hopefully somewhere in the next two weeks. |
|
I came here looking for exactly this, thank you. Another good thing to have for |
This makes the underlying storage type, used by the containers and previously hardcoded to
bits::carray, configurable.It then adds a specialized container (
pic_array) for storing variable-length arrays ofTs. The most prominent example of such arrays being strings (arrays ofchar). This container stores these arrays as sub-arrays inside one large storage array.This simultaneously achieves:
R_<arch>_RELATIVE-style load-time relocations (for-fPIC) because it stores relative addresses to data (indices into the storage array) instead of absolute addressesrodata) memory than storing pointers by selecting the smallest possible integer capable of addressing the entire storage array for storing relative addressesI have two different use cases where this benefits me significantly:
ld.so) is relocating large amounts of string pointers in.data.rel.ro: these get moved to.rodatawhich doesn't need to be relocated at all.Depends-on: #158