billyquith / ponder

C++ reflection library with Lua binding, and JSON and XML serialisation.
http://billyquith.github.io/ponder/
Other
642 stars 95 forks source link

needless memory allocations #11

Open seanmiddleditch opened 8 years ago

seanmiddleditch commented 8 years ago

Ponder seems to make many needless allocations. For a first example, let's look at the .property member of ClassBuilder:

 ClassBuilder<T>& ClassBuilder<T>::property(const std::string& name, F1 accessor1, F2 accessor2)

Note that it takes a std::string for the property name. Typical uses here will always be using a string literal in the reflection declaration, however. The official example:

ponder::Class::declare<Person>("Person")
    .property("name", &Person::name);

Here, "name" will be copied (with allocation, sans any small-string optimization). Allocating and copying static strings is mostly unnecessary.

This is even more problematic in your query functions, e.g. Class::hasProperty. In that case, the std::string const& parameter requires a temporary allocation just to query data; the allocation serves no other purpose than to make the public API take the C++ string type. This is a primary location where std::experimental::string_view would be a far more appropriate type (a ponder-specific string_view for users not running bleeding-edge C++ implementations works too, of course).

Another example of unnecessary allocation from ClassBuilder::base<>:

std::string baseName = baseClass.name();

The .name() member function of Class returns a std::string const& yet in this line of code you're creating a new local value type and copying/allocating the string. This should at least be a local std::string const& baseName, assuming you don't remove use of std::string entirely.

billyquith commented 8 years ago

Thanks for the feedback. Yes, that is something I'd like to deal with at some point. You get the same problem when declaring scripting (e.g. Lua) interfaces. Also, all of the string compares are not efficient.

This library is a fork of CAMP and other users attempted to fix this problem by using hashing, e.g. refer to https://github.com/cofenberg/camp/commit/5e382f930b61619843b0d83e299c61ca34c77ead

I forked from the source version though as I'd like to tackle the problem slightly differently. I was thinking of adding an "identifier trait" which could be overloaded depending on the project. I've previously used cached strings and hashes, which is much more efficient, and I recently found a nice implementation of this on GitHub: https://github.com/foonathan/string_id

I wasn't aware of string_view, thanks I'll keep an eye on that, a useful concept, and perhaps part of a future solution.

I'd also like to take a closer look at the allocations at some point, the number, and space used. I consider the library in "beta" at the moment. It "works" but needs a good testing in reasonably complex real project. Started doing this in Gwork: https://github.com/billyquith/GWork/tree/ponder

billyquith commented 8 years ago

This is implemented in the 1.3 branch. I have used traits for the identifier types so that the types may be changed in the future, e.g. EASTL, or string_id.

Thanks for the feedback.