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

std::unordered_map serializer causing huge memory explosion with empty container #180

Closed
lifflander opened this issue Mar 3, 2021 · 6 comments · Fixed by #181
Closed
Assignees

Comments

@lifflander
Copy link
Contributor

This code seems like it has a bug that is causing a huge number of unordered_map nodes to be allocated even though the target container is actually empty the entire time. It's causing a massive growth of hundreds of megabytes:

template <typename SerializerT, typename ContainerT>
inline void serializeUnorderedAssociativeContainer(
  SerializerT& s, ContainerT& cont
) {
  if (not s.isFootprinting()) {
    auto max_load_factor = cont.max_load_factor();
    s | max_load_factor;
    cont.max_load_factor(max_load_factor);

    auto bucket_count = cont.bucket_count();
    s | bucket_count;
    cont.rehash(bucket_count);
  }

  serializeMapLikeContainer(s, cont);
}

At the very least we should not be rehashing everything during sizing and packing! That has linear complexity.

Screen Shot 2021-03-03 at 2 25 31 PM

Screen Shot 2021-03-03 at 2 25 46 PM

@PhilMiller PhilMiller changed the title std::unordered_map serializer causing huge memory exposition with empty container std::unordered_map serializer causing huge memory explosion with empty container Mar 3, 2021
@PhilMiller
Copy link
Member

Does it work to stick a if (s.isUnpacking()) before the rehash call?

@lifflander
Copy link
Contributor Author

We can probably start by writing a unit test (@cz4rs) to try to reproduce this problem locally. I think we pushed the latest checkpoint code out to EMPIRE so we need to fix that ASAP.

@lifflander
Copy link
Contributor Author

Does it work to stick a if (s.isUnpacking()) before the rehash call?

I haven't tried that... I'm just removing the code altogether to ensure all the memory increase is due to this.

@cz4rs
Copy link
Contributor

cz4rs commented Mar 3, 2021

looking into it right now, I'll try isUnpacking after setting up the test

@lifflander
Copy link
Contributor Author

With those lines completely commented out the memory usage looks like this:

Screen Shot 2021-03-03 at 2 34 25 PM

@cz4rs
Copy link
Contributor

cz4rs commented Mar 4, 2021

@lifflander @PhilMiller I have creted a PR trying to call rehash only when necessary (s.isUnpacking() only cuts the calls number roughly in half).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants