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

MSVC performance issue with ArgMap #261

Closed
mwinterb opened this issue Jan 10, 2016 · 2 comments
Closed

MSVC performance issue with ArgMap #261

mwinterb opened this issue Jan 10, 2016 · 2 comments

Comments

@mwinterb
Copy link
Contributor

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:

  1. 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.
  2. 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.

@vitaut
Copy link
Contributor

vitaut commented Jan 10, 2016

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?

@vitaut
Copy link
Contributor

vitaut commented Jan 12, 2016

Fixed by #262. Thanks!

@vitaut vitaut closed this as completed Jan 12, 2016
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

No branches or pull requests

2 participants