DoseActor does not seems to work in MT anymore

[dsarrut]

:cry:

It means tests are not robust enough

[nkrah]

You are right. I was (knowingly) negligent about the mutexes in my consolidation PR. Knowingly because I think we will very quickly switch to atomic doubles.

[andiresch]

does it mean, that we need an additional and sensitive test case - as next step?

[nkrah]

we could, by I think the issue will be resolved within a week or so when we switch to atomic double implementation because that will make the actor intrinsically thread-safe. I have an old branch that I used for extensive tests this spring - I guess you remember. I reviewed it these days and it will be simple implement the changes in the current code. Based on that, we can put together a reasonable test that is maximally sensitive to concurrent writing in the dose map.

@andiresch : Do you have a test scenario on mind?

[andiresch]

When I changed to thread local images/1darray, low resolution images were the ones with most concurrency problems (ok that's trivial). I used to have a test case with multiple identical simulations (proton beam on water, same seed, number threads = number of avail cores ) but with different resolution. The sum of edep should be the same for all. Note: use a 1d resolution along beam direction to increase probability of problems. I can provide the test case coming week if you find it useful

[nkrah]

Yes, good idea. 1d dose profiles with large spacing and small step limiter to provoke concurrent writing in the same voxels.

On Oct 29 2024, at 7:28 am, andiresch @.***> wrote:

When I changed to thread local images/1darray, low resolution images were the ones with most concurrency problems (ok that's trivial). I used to have a test case with multiple identical simulations (proton beam on water, same seed, number threads = number of avail cores ) but with different resolution. The sum of edep should be the same for all. Note: use a 1d resolution along beam direction to increase probability of problems.

I can provide the test case coming week if you find it useful

—

Reply to this email directly, view it on GitHub (https://github.com/OpenGATE/opengate/issues/508#issuecomment-2443319422), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AIFQFYIURKXODQOXORRSLDLZ54TJZAVCNFSM6AAAAABQRE2NK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBTGMYTSNBSGI).

You are receiving this because you commented.

[BishopWolf]

You are right. I was (knowingly) negligent about the mutexes in my consolidation PR. Knowingly because I think we will very quickly switch to atomic doubles.

Are you using atomics library ? https://pypi.org/project/atomics/

atomicx seems better, at least it's upgraded more frequently. https://pypi.org/project/atomicx/

Naive bytes2double and double2bytes implementation using struct native python library

>>> d = 1.234
>>> b = struct.pack('d', d)
>>> b
b'X9\xb4\xc8v\xbe\xf3?'
>>> d2, = struct.unpack('d', b)
>>> d2
1.234

[nkrah]

Thanks for pointing to the libraries. We are using none of the two really because the code in question is in C++. Just plain atomic types from the std library.

[BishopWolf]

Thanks for pointing to the libraries. We are using none of the two really because the code in question is in C++. Just plain atomic types from the std library.

In this case, remember to guard the std::atomic operations in helper functions like this one

std::atomic<double> foo{0};

void add_to_foo(double bar) {
  auto current = foo.load();
  while (!foo.compare_exchange_weak(current, current + bar));
}

As a rule, never trust the implicit atomicity of your compiler, it may break badly in another compiler.

Refs: https://stackoverflow.com/questions/45055402/atomic-double-floating-point-or-sse-avx-vector-load-store-on-x86-64 https://stackoverflow.com/questions/23116279/how-to-perform-basic-operations-with-stdatomic-when-the-type-is-not-integral

To resume: pre c++20 compilers tends to do a really bad job linking the double atomicity to hardware. Remember, SSE4 only support integer types. For what I can see, you are still in c++17. All this is fixed in c++20.

[nkrah]

Yes, exactly. In our prototype we are using this kind of CAS loop.

[nkrah]

PR #520 fixes MT for dose-like actors via muteness (as before). I put the mutex statements into scopes to minimize their impact on other parts of the code.

[nkrah]

DoseActor, LETActor, FluenceActor, TLEDoseActor now have mutexes and are therefore thread-safe. I'll close the issue.