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

Fix 'minimum_memory_phase1' returns an illegal value #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SSoelvsten
Copy link
Contributor

@SSoelvsten SSoelvsten commented May 24, 2022

Closes #250 as a simpler (and working) alternative to #253 . All tests pass, and the computed value seems to be slightly above the min_m1 computed in calculate_parameters for a block size of 2, 4, 8, ..., 64. Except for the tests and my own project, Adiar, I do not know what to test it on.

@freekvw
Copy link
Collaborator

freekvw commented Jun 3, 2022

See #255. Not sure if this also solves your issue?

@SSoelvsten
Copy link
Contributor Author

SSoelvsten commented Jun 3, 2022

I tried to rerun the minimal example in #250 on main after the merge of #255 and I get

Not enough phase 1 memory for 128 KB items and an open stream! (25167632 < 25298776)

So, this function still seems to return a too small value. The computation in minimum_memory_phase1 is a separate code-duplication of the line you changed. Here, the only difference is the 128*1024 / m_item_size (now std::max(128*1024UL, 16*m_item_size)) is replaced with a 1 which is very unlikely to be the same.

@SSoelvsten SSoelvsten force-pushed the merge_sorter/min_memory_available branch 3 times, most recently from 25adb86 to 06a8a35 Compare June 3, 2022 13:05
@SSoelvsten SSoelvsten force-pushed the merge_sorter/min_memory_available branch from 06a8a35 to d587f0d Compare March 8, 2023 16:17
@SSoelvsten SSoelvsten force-pushed the merge_sorter/min_memory_available branch from d587f0d to 742f060 Compare March 9, 2023 15:37
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

Successfully merging this pull request may close these issues.

merge_sorter::minimum_memory_phase_X() is lower than actual memory requirements
2 participants