You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While looking into #193, I was surprised to see std::map memory allocation related operations high up in the profile. It turned out that this was because ArgMap is a member of BasicFormatter, ArgMap has an std::map<> member that's default constructed, and in MSVC's std library implementation, all of map's constructors allocate a root node1. libc++ and libstdc++ do not, at first glance, which is probably why this hasn't been seen already (I don't think anyway, no closed issues with ArgMap popped up in a search).
Options I see for addressing this:
Defer construction of the std::map in ArgMap until it is initialized. Having something like boost::optional would be nice here, but it's not necessary.
Instead of using a map for the backing store, use a vector. I think all three major std library implementations have a non-allocating default constructor. The vector may not even need to be sorted, as it doesn't look like anything depends on order or even uniqueness and there probably isn't going to be a large number of named arguments. If it does, it's trivial to maintain that constraint since the map is effectively immutable after init() is called.
However, both of those options would break binary compatibility.Option 2 seems like the cleanest option.
I've done a prototype with option 2, and it seemed to improve #193 from ~1.5s to ~0.9s, roughly in line with the results you're currently getting.
1 The rationale for why it does this is described in this paper, just search for Lavavej. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4055.html
While it's in the context of a move constructor, and list, the point being made applies equally to the default constructor and other node-based containers.
The text was updated successfully, but these errors were encountered:
Replacing a map with a vector looks like a good idea. In fact, I was planning to do something along these lines, but didn't have time for it yet. The binary compatibility will be broken in both options, but I think it's worth it. We'll just have to bump the major version number and it might take a while until this is released.
Since you've done some work in this direction already, could you by any chance submit a PR?
While looking into #193, I was surprised to see std::map memory allocation related operations high up in the profile. It turned out that this was because ArgMap is a member of BasicFormatter, ArgMap has an std::map<> member that's default constructed, and in MSVC's std library implementation, all of map's constructors allocate a root node1. libc++ and libstdc++ do not, at first glance, which is probably why this hasn't been seen already (I don't think anyway, no closed issues with ArgMap popped up in a search).
Options I see for addressing this:
However, both of those options would break binary compatibility.Option 2 seems like the cleanest option.
I've done a prototype with option 2, and it seemed to improve #193 from ~1.5s to ~0.9s, roughly in line with the results you're currently getting.
1 The rationale for why it does this is described in this paper, just search for Lavavej.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4055.html
While it's in the context of a move constructor, and list, the point being made applies equally to the default constructor and other node-based containers.
The text was updated successfully, but these errors were encountered: