thomasmoelhave / tpie

Templated Portable I/O Environment
Other
112 stars 24 forks source link

tpie::priority_queue(mm_avail) not in documentation #244

Open SSoelvsten opened 3 years ago

SSoelvsten commented 3 years ago

There is a #ifndef DOXYGEN around the absolute memory constructor for tpie::priority_queue. Its implementation seems to not have anything dangerous in it since it starts with the following assertion.

assert(mm_avail <= get_memory_manager().limit() && mm_avail > 0);

Is it ill advised to use this function and hence should it not be part of the documentation?

SSoelvsten commented 3 years ago

My use my case/motivation is in the many algorithms of the BDD package Adiar that include lines close to the following:

// open `tpie::file_stream` for input and handle edge cases
. . .

// Init priority queues
tpie::memory_size_type available_memory = tpie::get_memory_manager().available();

tpie::priority_queue<...> pq_1(available_memory / 3);
tpie::priority_queue<...> pq_2(available_memory / 3);
tpie::priority_queue<...> pq_3(available_memory / 3);

// The actual algorithm
. . .

Initialising one after the other with a mere factor of 0.333 does not use all available memory as far as I can tell. I'd argue that writing the factors relative to each other (0.333, 0.5, 1.0) is not easy to understand/maintain. It especially becomes complicated, as pq_1 usually is a custom adiar::levelized_priority_queue that uses multiple tpie::merge_sorters and a single tpie::priority_queue (and hence needs to juggle both absolute and fractional memory).

Mortal commented 3 years ago

The constructor was hidden in commit f1d4d926b024c6dc91cdcec690fac582031e267f as part of a bigger commit that "fixes the documentation", whatever that means. I'm guessing I had problems getting doxygen to run, so I just hid it as a workaround. If there aren't any issues with building the documentation, then I think it's fine to just add it.

SSoelvsten commented 3 years ago

I didn't think of putting this in could break compiling the documentation, so I have not tested that. I'll double check the ability to build the documentation.