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

crash on reserve(1) #26

Closed
kilobyte opened this issue Jun 3, 2019 · 12 comments
Closed

crash on reserve(1) #26

kilobyte opened this issue Jun 3, 2019 · 12 comments

Comments

@kilobyte
Copy link

kilobyte commented Jun 3, 2019

If preallocation via reserve() requests only 1 or 2 elements, there's either a crash — or, in debug builds, an assert failure.

@mattreecebentley
Copy link
Owner

Minimum size for reserve is same as minimum size for groups in colony (3), so this is intentional. Will update the documentation.
It makes little sense for anything but very large elements to have groups that can contain only 2 elements.

@kilobyte
Copy link
Author

kilobyte commented Jun 4, 2019

Yeah but that'll break when the size being reserved comes from an outside source — like, a data file being read having only 1 or 2 elements. That's a surprising crash for any developer using the library.

I'd propose doing it like STL does, silently increasing the size. You already do so for shrink_to_fit(): for 4-byte elements on a 64-bit arch, after a single insert() then shrink_to_fit() the capacity is 72.

@mattreecebentley
Copy link
Owner

It already does so.
It bumps up to the min group size, or down to the max group size.
The crash is unexpected so I'll check that out.

@mattreecebentley
Copy link
Owner

I'm going to remove the asserts so reserve will silently accept bad input, but I
can't replicate the crash when under-supplying the reserve function with a reserve count of 2, could you look into that ?

@kilobyte
Copy link
Author

kilobyte commented Jun 5, 2019

I found this as a part of a bigger program — and indeed I can't reproduce the crash (as opposed to assert failure) with a minimal test case either. Can't really do debugging right now — got 4 hours of sleep before a flight.

@mattreecebentley
Copy link
Owner

All good, if you find the source of the crash do let me know.

@kilobyte
Copy link
Author

kilobyte commented Jun 5, 2019

Debugged this on the plane, turns out the crash was actually my fault: plf::colony is not supposed to be thread-safe so that's ok. Bizarrely, the only reserve() call in the program was for thread pool, so running with -j1 made the crash and the assert mutually exclusive :)

As for the assert: it's you who gets to declare what's legal with plf::colony's API, but as 1. STL allows too small reserves (which gcc's implementation silently enlarges, like you do with shrink_to_fit()), and 2. it's surprising to the user -- I'd urge you to handle such small reserves some way or another (be it by silently enlarging, or dropping the assert).

@mattreecebentley
Copy link
Owner

As I've said, I intend to drop the asserts and so rely on the user to investigate the documentation properly when memory sizes don't match their expectations, but neither solution makes me happy.
I'd rather be Throw'ing if the user calls reserve with values outside of the parameters.

Good to hear the crash isn't my fault :) I would love it if someone made a thread-safe version, I don't have enough experience.

@kilobyte
Copy link
Author

kilobyte commented Jun 5, 2019

Could you please make reserve() accept plausible values like 1 (doing something useful, not necessarily exact allocation)?

No idea about thread-safe but some folks at work are taking your ideas and plan/toy with plf::colony on persistent memory. This requires some strong discipline about flushing memory writes, doing allocations in atomic or transational way, handling unexpected power loss at any moment without data loss or corruption. This is how I learned about your library -- for now, I'm using it for a personal non-work-related project.

@mattreecebentley
Copy link
Owner

As I've said, I intend to drop the asserts and silently accept lower-than-possible values. It already rounds up to the minimum group size.

@mattreecebentley
Copy link
Owner

Changes committed

@mattreecebentley
Copy link
Owner

ps. Let me know if your work-folk have any success in persistent memory - would be interesting to hear -

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