Added GEANT4_BUILD_MULTITHREADED env variable: #1189

ihrivnac · 2018-06-12T12:22:03Z

The variable allows different setting of the GEANT4_BUILD_MULTITHREADED CMake option
The default value "OFF" is overridden ("ON") in defaults-o2.sh and defaults-o2-dev-fairroot.sh

dberzano · 2018-06-12T12:30:13Z

geant4.sh

@@ -25,6 +25,10 @@ env:
 ---
 #!/bin/bash -e

+if [ ! $GEANT4_BUILD_MULTITHREADED ]; then


Change those three lines to:

: {GEANT4_BUILD_MULTITHREADED:=OFF}

which does exactly the same. (Mind the : at the beginning of the line.)

geant4.sh

sawenzel · 2018-07-03T06:51:48Z

I see not activity here. @ihrivnac : I think this can be merged as soon as the suggestions of @dberzano have been addressed.

sawenzel · 2018-08-23T13:38:45Z

@ihrivnac, @dberzano: I have updated this PR according to the suggestions (since there was no activity for a while). I think we should go on with this.

Please squash the commits together.

dberzano

A small glitch to fix. Can you also add the variable to o2-ninja please? Thanks!

dberzano · 2018-08-23T13:46:04Z

geant4.sh

-  -DGEANT4_USE_SYSTEM_EXPAT=OFF                     \
-  ${CXXSTD:+-DCMAKE_CXX_STANDARD=$CXXSTD}           \
+# if this variable is not defined default it to OFF
+: {GEANT4_BUILD_MULTITHREADED:=OFF}


This should be:

: ${GEANT4_BUILD_MULTITHREADED:=OFF}

(a $ is missing).

sawenzel · 2018-08-23T14:01:14Z

Actually I can't modify/edit o2-ninja.sh in relation to this PR (on this very branch). I would prefer doing it in a sep PR directly hereafter.

need to update branch; review is blocking update

dberzano · 2018-08-23T14:06:00Z

Hm I don't understand why you can't edit it. Let me check.

dberzano · 2018-08-23T14:07:51Z

OK I just did it.

sawenzel · 2018-09-12T07:40:12Z

What is the status here? The CI test seems to be actually failing.

sawenzel · 2018-09-18T16:40:53Z

Can this be merged, please?

ktf · 2018-09-19T06:48:31Z

Aren’t the failures related?

ktf · 2018-09-19T06:49:39Z

Also, could you please rebase it? There are conflicts.

dberzano · 2018-10-01T16:14:16Z

This is ready to be merged at your convenience. I cannot positively review it because I have contributed to it.

@sawenzel, @ktf if you agree can you please review it and merge when green?

@ihrivnac: I had to rebase it once again for the record 🙂

ktf

There seems to be a lot of spurious messages / errors from what seems to be MT related. Are you really sure you want to merge this?

dberzano · 2018-10-01T20:23:15Z

I am not, but @sawenzel seems to need it. Maybe him or @ihrivnac can shed some light on the error messages?

ihrivnac · 2018-10-02T09:43:39Z

There seems to be a problem with switching the GEANT4_BUILD_TLS_MODEL option from "global-dynamic" to "initial-exec"; this option was set to get working the dynamical loading of libraries via ROOT gSystem when running in MT mode, however it brought an overhead (~3%) when running in sequential mode with libraries built with MT enables.
In our local test, it looked that this option is not needed anymore, but from the tests here it looks like it is still needed for certain platforms. I suggest to try everything with the TLS model "global-dynamic". I will commit the update in my branch.
Could you let me know what's the platform (os, compiler) which is used for the PR checks?

dberzano · 2018-10-02T09:58:05Z

CC7, GCC v7.3.0

dberzano · 2018-10-02T09:58:41Z

Note that we can selectively enable it on certain platforms if you tell us so.

ihrivnac · 2018-10-02T10:19:03Z

The platforms which we tested locally were:
Ubuntu:16.04 (Sandro), MAC OS HIgh Sierra 10.13.4 (me)
So we may need a selective setting here.

sawenzel · 2018-10-02T11:32:04Z

Fine. But if the production environment has this problem it means that we are back to accepting a 3% penalty just by theoretically enabling this mode (without even using it). At least I have a problem with this. Is there no other way to avoid this? How are other experiments solving this issue?

ktf · 2018-10-02T11:40:06Z

CMS uses global-dynamic:

https://github.com/cms-sw/cmsdist/blob/IB/CMSSW_10_3_X/gcc700/geant4.spec

not sure if they have anything extra in the framework itself.

Barthelemy

I approve but I have no idea on the subject :)

sawenzel · 2018-10-02T12:16:31Z

For the record, the reasoning to put initial-exec in commit 86ed6651973a56db can be found here: https://alice.its.cern.ch/jira/browse/O2-172.

sawenzel · 2018-10-02T12:22:25Z

I note that even with global-dynamic the test crashes and has tons of warnings.

ihrivnac · 2018-10-02T12:55:42Z

In the new log, only the new test o2sim_G4_mt fails, and it happens at exit, so there is some problem with final clean-up, which I will investigate.
On the other hand, according to Geant4 banner, the Geant4 libraries were not rebuilt with changing the build option; also, I don't see in the log if geant4_vmc libraries were re-built. Is it possible to check this on the test machine?
The large number of warning come from FairRoot framework:
### [WARN] The method CheckIfSensitive has to be implemented in the detector class which
inherits from FairModule
and it is not relevant to this PR. I'd expect that they are present in the Passed tests, too. Could we check the logs of o2sim_G3 and o2sim_G4?

ihrivnac · 2018-10-03T08:33:51Z

I proposed a fixed for the warnings in O2 PR #1382. With introducing the new function InitializeO2Detector() in O2 there was disconnected the initialisation of sensitive volumes, triggered by DefineSensitiveVolumes() in FairRoot.
On the other hand I am not able to reproduce the break at the end of run. Is it possible to produce a back trace from this crash on the test machine?

- The variable allows different setting of the GEANT4_BUILD_MULTITHREADED CMake option - The default value "OFF" is overriden ("ON") in defaults-o2.sh and defaults-o2-dev-fairroot.sh

dberzano · 2018-10-09T19:48:32Z

@ihrivnac there is something systematically wrong in your Git workflow (you keep bringing here commits that don't belong and are already in master).

If you want to align this pull request to the changes made upstream, simply press the Update branch button on the GitHub interface
If you want to do the same but manually, simply git pull --rebase origin master && git push -f when you are on your geant4_mt branch

Hope this helps!

ihrivnac · 2018-10-09T20:11:16Z

Thanks. I clicked on "Update branch" in github, and got again the red button:
"Merging is blocked" ...

dberzano · 2018-10-09T20:15:58Z

Yes, merging is blocked because there is a negative review. Were the comments from @ktf addressed?

ktf · 2018-10-09T20:33:55Z

At leasts all the tests are green now, however there is no way to check if the spurious messages. Didn't we copy successful test logs? That said, no more objections as long as things are green.

dberzano · 2018-10-09T21:06:57Z

Successful logs are not retained ATM.

sawenzel · 2018-10-10T05:59:31Z

I would have preferred resolving the performance problem before merging. Will re-open https://alice.its.cern.ch/jira/browse/O2-172 since we are back to global-dynamic threading which has runtime overhead.

ihrivnac requested a review from a team as a code owner June 12, 2018 12:22

dberzano suggested changes Jun 12, 2018

View reviewed changes

dberzano previously requested changes Aug 23, 2018

View reviewed changes

dberzano force-pushed the geant4_mt branch from 7d94074 to 2d78cf3 Compare August 23, 2018 14:07

dberzano previously approved these changes Aug 23, 2018

View reviewed changes

ihrivnac dismissed dberzano’s stale review via 72b13d5 October 1, 2018 15:31

ihrivnac requested a review from a team as a code owner October 1, 2018 15:31

dberzano force-pushed the geant4_mt branch from 4682acd to 1daf2c5 Compare October 1, 2018 16:10

ktf self-requested a review October 1, 2018 19:11

ktf requested changes Oct 1, 2018

View reviewed changes

Barthelemy previously approved these changes Oct 2, 2018

View reviewed changes

ihrivnac and others added 6 commits October 9, 2018 10:15

Added GEANT4_BUILD_MULTITHREADED env variable:

7ea5961

- The variable allows different setting of the GEANT4_BUILD_MULTITHREADED CMake option - The default value "OFF" is overriden ("ON") in defaults-o2.sh and defaults-o2-dev-fairroot.sh

implement comments raised by review

6fef438

fix syntax error

5eabc6f

Add GEANT4 MT option to o2-ninja

53830ea

Switch GEANT4_BUILD_TLS_MODEL to global_dynamic

b1fe9b1

Fix typo: global_dynamic -> global-dynamic

c660418

ihrivnac dismissed Barthelemy’s stale review via c660418 October 9, 2018 08:30

ihrivnac force-pushed the geant4_mt branch from f70266f to c660418 Compare October 9, 2018 08:30

ihrivnac and others added 2 commits October 9, 2018 10:36

Removed redundant definition

25c9a76

Update readoutcard.sh (alisw#1323)

1da2d1c

Merge branch 'master' into geant4_mt

b6b21f9

ktf approved these changes Oct 9, 2018

View reviewed changes

dberzano merged commit 137b1c3 into alisw:master Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added GEANT4_BUILD_MULTITHREADED env variable: #1189

Added GEANT4_BUILD_MULTITHREADED env variable: #1189

ihrivnac commented Jun 12, 2018

dberzano Jun 12, 2018

sawenzel commented Jul 3, 2018

sawenzel commented Aug 23, 2018 •

edited

Loading

dberzano left a comment

dberzano Aug 23, 2018

sawenzel commented Aug 23, 2018

dberzano commented Aug 23, 2018

dberzano commented Aug 23, 2018

sawenzel commented Sep 12, 2018

sawenzel commented Sep 18, 2018

ktf commented Sep 19, 2018

ktf commented Sep 19, 2018

dberzano commented Oct 1, 2018

ktf left a comment

dberzano commented Oct 1, 2018

ihrivnac commented Oct 2, 2018 •

edited

Loading

dberzano commented Oct 2, 2018

dberzano commented Oct 2, 2018

ihrivnac commented Oct 2, 2018

sawenzel commented Oct 2, 2018

ktf commented Oct 2, 2018

Barthelemy left a comment

sawenzel commented Oct 2, 2018

sawenzel commented Oct 2, 2018

ihrivnac commented Oct 2, 2018

ihrivnac commented Oct 3, 2018

dberzano commented Oct 9, 2018

ihrivnac commented Oct 9, 2018

dberzano commented Oct 9, 2018

ktf commented Oct 9, 2018

dberzano commented Oct 9, 2018

sawenzel commented Oct 10, 2018

Added GEANT4_BUILD_MULTITHREADED env variable: #1189

Added GEANT4_BUILD_MULTITHREADED env variable: #1189

Conversation

ihrivnac commented Jun 12, 2018

dberzano Jun 12, 2018

Choose a reason for hiding this comment

sawenzel commented Jul 3, 2018

sawenzel commented Aug 23, 2018 • edited Loading

dberzano left a comment

Choose a reason for hiding this comment

dberzano Aug 23, 2018

Choose a reason for hiding this comment

sawenzel commented Aug 23, 2018

dberzano commented Aug 23, 2018

dberzano commented Aug 23, 2018

sawenzel commented Sep 12, 2018

sawenzel commented Sep 18, 2018

ktf commented Sep 19, 2018

ktf commented Sep 19, 2018

dberzano commented Oct 1, 2018

ktf left a comment

Choose a reason for hiding this comment

dberzano commented Oct 1, 2018

ihrivnac commented Oct 2, 2018 • edited Loading

dberzano commented Oct 2, 2018

dberzano commented Oct 2, 2018

ihrivnac commented Oct 2, 2018

sawenzel commented Oct 2, 2018

ktf commented Oct 2, 2018

Barthelemy left a comment

Choose a reason for hiding this comment

sawenzel commented Oct 2, 2018

sawenzel commented Oct 2, 2018

ihrivnac commented Oct 2, 2018

ihrivnac commented Oct 3, 2018

dberzano commented Oct 9, 2018

ihrivnac commented Oct 9, 2018

dberzano commented Oct 9, 2018

ktf commented Oct 9, 2018

dberzano commented Oct 9, 2018

sawenzel commented Oct 10, 2018

sawenzel commented Aug 23, 2018 •

edited

Loading

ihrivnac commented Oct 2, 2018 •

edited

Loading