Move the LogType check inside WriteStats

[jodavies]

This allows other code to be included in the lock, in particular to avoid race conditions when temporarity setting AC.LogHandle = -1 when AM.LogType is 1.

Fixes #470

[jodavies]

Given the large output of helgrind and drd in https://github.com/vermaseren/form/issues/470#issuecomment-1996014250 it might be an idea to make a careful study of potential problems, but changes due to that probably belong in a separate PR.

[tueda]

it might be an idea to make a careful study of potential problems

Indeed, I was trying to resolve Helgrind errors, here, still have a long way to go. This is very tricky even for a simple program like

L F = 1;
.end

For example, this commit fixes a Helgrind error by introducing a barrier, but it leads to another drd error about barrier destruction (which must be fixed).

[tueda]

I think it's OK to merge, but I have 2 concerns:

• Possible performance loss (negligible?): WriteStats() has many early returns, where nothing is written, so the lock is unnecessary for these cases.
• Do we need to use the lock for WriteStats() in the master thread (2 places in threads.c)? Is it logically fine without the lock after considering interferences with other locks and the program flow (too complicated to follow), or might it be OK in practice (though logically bad)?

I'm not sure but maybe the juggling of AC.LogHandle could be moved into WriteStats(), possibly at the cost of adding another argument (??)

[jodavies]

With typical buffer sizes, printing of stats is supposed to be very rare compared to actual algebra processing. I would say that if these locks become a bottleneck, far more performance improvement is available to the user by adjusting buffer sizes, than by locking a bit less (after the early returns, for eg). But indeed, if an argument is added, the lock can go back inside WriteStats. This is quite an easy change, WriteStats is not called very many times across the codebase.

For your second point, I don't know. I just made sure that if there were locks before this change, there are also locks after.

[jodavies]

Your suggestion is implemented in 96f040d.

This is much cleaner I think.

A possible additional cleanup: make defines for par={0,1,2} for the second argument of WriteStats ?

[tueda]

This is much cleaner I think.

Very nice, thanks!

A possible additional cleanup: make defines for par={0,1,2} for the second argument of WriteStats ?

I think this is a good idea for readability.

[jodavies]

I think now that your comment regarding performance of the original commit was important. Since in WriteStats everything is inside if ( AT.SS == AT.S0 && AC.StatsFlag ) {, the original commit introduced unnecessary locking for all of the higher level sorts (of function arguments, etc) which never print any stats.

I'll squash these, if you are happy with the current state.

[coveralls]

coverage: 48.493% (+0.004%) from 48.489% when pulling 5c6abd3ce936fc551910c9f163351c0ac79b376e on jodavies:issue-470 into 3ccda3a3145036a7a8def3ae3bf7b3dbe8066646 on vermaseren:master.

[tueda]

Merged. Thanks!