GeometrySplit functor isn't threadsafe

[simonge]

Briefly, what does this PR introduce?

Fixes issues with the initialization of the GeometrySplit functor across threads.

Currently only the std::once_flag is a shared pointer and none of the other variables which are being initialized. This meant that when copied to other threads the is_init flag was set but the decoder and geometry divisions had not been so the loop over the detector ids was always empty. This resulted in an apparent variable decrease in the efficiency of the Low-Q2 tagger depending on how many threads were asked for.

Unit tests for all of the meta factories and functors should be added as an issue to try head this off in the future but outwith this fix.

What kind of change does this PR introduce?

• Bug fix (issue #__)
• New feature (issue #__)
• Documentation update
• Other: __

Please check if this PR fulfills the following:

• Tests for the changes have been added
• Documentation has been added / updated
• Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

All of the Low-Q2 Tagger hits will be converted into clusters rather than just those run on the first thread to call the functor.

[veprbl]

info might be a bit too loud

[simonge]

There was an alternative solution, overwriting the copy constructors for the class so that each copy has a new std::once_flag and does it's own initialization on the first call. Making everything needing initialized shared pointers feels cleaner.

[simonge]

Are any of the CI tests run on multiple threads which would be possible to compare the output between a single and multi-threaded run?

[simonge]

Are any of the CI tests run on multiple threads which would be possible to compare the output between a single and multi-threaded run?

Looks like this was being partially approached in #975. For cases like this having at least one job still run on a single core to compare the output would be useful.

Checking the default flags it appears nthreads should be 1 if not specified, however the behaviour change seen here was only identified by explicitly adding the -Pnthreads=1 flag. Doesn't sound like that should have made a difference, any idea why it did?

[github-actions]

Capybara summary for PR 1607

• rec_dis_10x100_minQ2=0_craterlake
• rec_dis_10x100_minQ2=1000_craterlake_tracking_only
• rec_dis_18x275_minQ2=0_craterlake
• rec_dis_18x275_minQ2=1000_craterlake
• rec_dis_5x41_minQ2=0_craterlake
• rec_e_1GeV_20GeV_craterlake
• rec_pi_1GeV_20GeV_craterlake
  ^{_{Last updated 2024-09-11T06:15-04:00 e8c1df8}}

[wdconinc]

Checking the default flags it appears nthreads should be 1 if not specified, however the behaviour change seen here was only identified by explicitly adding the -Pnthreads=1 flag. Doesn't sound like that should have made a difference, any idea why it did?

@nathanwbrei may be the best person to address that.

[simonge]

Please could this be reviewed independently of the issues around running/testing with different numbers of threads, I can flag that as separate issue.

[veprbl]

Do we understand why these are copied in each thread in the first place?

[simonge]

Do we understand why these are copied in each thread in the first place?

I'm not familiar enough with JANA2 resource management to comment but would be keen to hear an explanation.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[simonge]

If possible could this be reviewed and merged before tagging for September, the Low-Q2 Tagger reconstruction is (variably) broken without it. The questions raised around the threads and JANA will still need addressing but can be answered later.

[veprbl]

I get the idea, but not the details. Fine to merge IMO.