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

Set tpie::tempname before calling tpie_init() #694

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

nhusung
Copy link
Contributor

@nhusung nhusung commented Oct 5, 2024

This avoids an additional directory named TPIE_<date>_<…> at the default tmp path.

About a year ago, when executing benchmarks in a scripted way, I ran into the problem that the BDDs produced by Adiar filled up the entire disk. Because of that, the benchmarking process crashed and left the files there, so my benchmark runner script was not even able to create new log files for the upcoming benchmarks. That was when I started to use the -T option in BDD benchmark to make Adiar write to a location I previously created using Python’s tempfile.TemporaryDirectory(), so I could be sure that would be cleaned even upon timeouts and crashes.

However, that fix seemed to be ineffective, Adiar still created directories named TPIE_<date>_<…> in my /tmp folder. So I added two additional lines to my benchmark running script to remove /tmp/TPIE_* after every benchmark item. Recently, I’ve switched to parallel execution of benchmarks, and it just came to my mind that removing all /tmp/TPIE_* might also remove the folders of another Adiar process, which could lead to crashes. Fortunately, there were no such crashes, but it made me think I should fix that issue in my script.

Seeking for solutions, I noticed that Adiar sets the temporary directory of TPIE only after initializing TPIE. So the /tmp/TPIE_* directories were probably created during the initialization, and afterwards TPIE switched to the other location. I just moved the initialization after setting TPIE’s temporary directory, and it worked as expected.

For sure, you know a lot more than me about TPIE, so please re-check that this change does not introduce any unwanted side-effects.

(EDIT: This is not urgent at all, it seems like the /tmp/TPIE_* directories stay below 1 MiB in size each, so I could just drop the removal of these directories from my script.)

Copy link
Owner

@SSoelvsten SSoelvsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code of TPIE, tpie::set_default_path sets a global std::string variable. This seems fully independent of tpie_init(...). I see no reason changing the order should break anything.

It also makes sense, that TPIE would forget its previous temporary library after changing it (except for the log file, which presumably already had open). Hence, it would forget to remove this directory. It should be noted though, that if compiled for production, TPIE should not be logging anymore since #692 (or maybe it creates the file and then that is it).

src/adiar/adiar.cpp Outdated Show resolved Hide resolved
@SSoelvsten
Copy link
Owner

SSoelvsten commented Oct 7, 2024

Recently, I’ve switched to parallel execution of benchmarks, and it just came to my mind that removing all /tmp/TPIE_* might also remove the folders of another Adiar process, which could lead to crashes. Fortunately, there were no such crashes, but it made me think I should fix that issue in my script.

Depending on what you are measuring, this may introduce a non-representative noise. One process may throw out the data needed by another process. That is, by measuring running time in parallel, you are introducing noise.

You should also notice something similar with Sylvan (and presumably OxiDD and LibBDD): as you increase the number of threads, you gain speed up to a (small) constant of the number of cores on your machine. Beyond doing so, performance decreases due to threads tripping over eachother.

This avoids an additional directory named `TPIE_<date>_<…>` at the
default tmp path.
@SSoelvsten SSoelvsten merged commit 6b17038 into SSoelvsten:main Oct 7, 2024
41 of 55 checks passed
@nhusung nhusung deleted the tpie-tmp-dir branch October 7, 2024 12:51
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.

2 participants