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

Improve saving geometry to root file #62

Closed
lobis opened this issue Jul 11, 2022 · 3 comments · Fixed by #58
Closed

Improve saving geometry to root file #62

lobis opened this issue Jul 11, 2022 · 3 comments · Fixed by #58
Assignees

Comments

@lobis
Copy link
Member

lobis commented Jul 11, 2022

Currently the logic to save the TGeoManager into the root file is the following:


    ////////// Writing the geometry in TGeoManager format to the ROOT file
    ////////// Need to fork and do it in child process, to prevent occasional seg.fault
    pid_t pid;
    pid = fork();
    if (pid < 0) {
        perror("fork error:");
        exit(1);
    }
    // child process
    if (pid == 0) {
        // writing the geometry object
        freopen("/dev/null", "w", stdout);
        freopen("/dev/null", "w", stderr);

        REST_Display_CompatibilityMode = true;

        // We wait the father process ends properly
        sleep(5);

        // Then we just add the geometry
        auto file = new TFile(filename, "update");
        TGeoManager* geoManager = gdml->CreateGeoManager();

        file->cd();
        geoManager->SetName("Geometry");
        geoManager->Write();
        file->Close();
        exit(0);
    }
    // father process
    else {
        int stat_val = 0;
        pid_t child_pid;

        printf("Writing geometry ... \n");
    }

I find this overly complex for what it is trying to achieve, I think we should simplify it.

In principle something similar to the following code could be used:

    TGeoManager* geoManager = gdml->CreateGeoManager();
    if (geoManager) {
        geoManager->Write("Geometry");
        delete geoManager;
    } else {
        cout << "Error: could not create geometry" << endl;
    }

However if this change is implemented, ocassional segfaults will appear, as the description on the current implementation describes.

I think we should fix the underlying issue which is likely on the core repository.

Perhaps @nkx111 which I think authored the original implementation can describe how it works, and why a fork is necessary.

@juanangp or @jgalan perhaps you also have some insights on this.

@juanangp
Copy link
Member

Can you try to remove delete geoManager; it might happen that you erase the pointer while it is still trying to write it, also memory management of root is not clear to me.

@lobis
Copy link
Member Author

lobis commented Jul 11, 2022

Can you try to remove delete geoManager; it might happen that you erase the pointer while it is still trying to write it, also memory management of root is not clear to me.

I tried to remove it and it didn't appear to have any effect. root memory management is not clear to me either but I think the problem is on our end, since I have used this same logic to save geometry in another project (https://github.com/lobis/radiation-transport/blob/main/applications/simulation/src/GlobalManager.cpp#L144) and it works without segfaults.

@lobis lobis linked a pull request Jul 29, 2022 that will close this issue
@nkx111
Copy link
Member

nkx111 commented Sep 4, 2022

Perhaps @nkx111 which I think authored the original implementation can describe how it works, and why a fork is necessary.

To some unknown reason the final step saving geometry to root file will crash occasionally. It happens randomly with about 3% probablity. So I added this fork step, to protect the main process, and make the pipeline always green.

@lobis lobis closed this as completed in #58 Sep 14, 2022
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 a pull request may close this issue.

3 participants