Remove all 'exit' stmts, use 'throw exception' instead

[ab-chesspad]

The main goal is to build SF as a shared library, so it can be used without restrictions on Android and other platforms.
This is the 1st commit to implement the idea that a server or a method should never crash or stop without the explicit request from the caller.

[lucasart]

As far as I know, uncaught C++ exceptions cause the program to terminate, same as calling exit(EXIT_FAILURE). I don't think you can launch SF from parent process (like a GUI for example), and have the parent process catch the exceptions launched by SF.

[ab-chesspad]

@lucasart You are right, but:

1. The main difference is that 'exit' terminates the whole program immediately, while exception can be caught and the calling code can decide what to do with the error.
2. When I submitted my full changes 3 weeks ago, people thought that they were too invasive and recommended me to split them into smaller, more manageable parts. So this is only the 1st one and yes, I intend to catch exceptions.

[vondele]

I've triggered another round of CI, previously it was failing. Not sure if that failure is related.

This change would need testing on fishtest to see if there is a performance impact of enabling exceptions. Personally, I think it is natural to have them enabled for C++ code, it is for example also needed for the cluster branch.

[ab-chesspad]

Hi Joost, if I get it right, the test fails on Mac OSX 10.5. 1. Is there a special reason to use the old (and obsolete?) version? I am currently using 11.6. 2. How can I see the log file of the failed build? Regards Alexander Bootman

…

On Mon, Nov 1, 2021 at 10:21 AM Joost VandeVondele ***@***.***> wrote: I've triggered another round of CI, previously it was failing. Not sure if that failure is related. This change would need testing on fishtest to see if there is a performance impact of enabling exceptions. Personally, I think it is natural to have them enabled for C++ code, it is for example also needed for the cluster branch. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF4K74NCWPMWYINBPA3PCUTUJ2O5BANCNFSM5G6CKRVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[vondele]

the test passed now after the restarting CI. I think it was a CI hickup, maybe timeout. The logs (one can see by clicking on the CI details link) showed it failed in the 'reprosearch' part, but that can hardly be influenced by this patch.

I think next test is run this on fishtest verifying no regression.

[lucasart]

Probably begnign, but wouldn't this introduce scrambled output? If a thread is doing a bunch of sync_cout / sync_endl (holding a custom mutex), while the compiler generated code handles uncaught exceptions by printing them to stderr (or stdout?), hence to screen in a terminal, even if it does hold a mutex for that (does it?), it will be a different mutex. Now, we're talking about a fatal error case, so perhaps scrambled output there is fine.

To be pedantically correct, perhaps we should catch the exceptions in main, and print the error message using sync_cout.

[ab-chesspad]

As I see it, there will be 2 mutexes, one for stdout, the other for stderr. All stderr messages are sent between one sync_cout / sync_endl, so there will be no or minimal scrambling. If the calling program chooses to merge stdout and stderr output, it should provide additional synchronization. On the other hand, exceptions should be caught on the command level, like 'go', so I do not foresee any output scrambling. I am preparing the complete update for my Android program, ChessPad. You will be able to have it once I am done. Regards Alexander Bootman

…

On Mon, Nov 1, 2021 at 8:57 PM lucasart ***@***.***> wrote: Probably begnign, but wouldn't this introduce scrambled output? If a thread is doing a bunch of sync_cout / sync_endl (holding a custom mutex), while the compiler generated code handles uncaught exceptions by printing them to stderr (or stdout?), hence to screen in a terminal, even if it does hold a mutex for that (does it?), it will be a different mutex. Now, we're talking about a fatal error case, so perhaps scrambled output there is fine. To be pedantically correct, perhaps we should catch the exceptions in main, and print the error message using sync_cout. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF4K74IXGKVWFDJZB4LHI63UJ4ZQPANCNFSM5G6CKRVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[dsmsgms]

Is it having exceptions necessary for the Stockfish program itself, I don't like it at all

[ddobbelaere]

As far as I understand, the so-called zero-cost implementation used in modern compilers (and ABIs) implies no runtime overhead as long as no exception is thrown.

See e.g. https://www.google.com/amp/s/mortoray.com/2013/09/12/the-true-cost-of-zero-cost-exceptions/amp/

[lucasart]

The hypothetical cost of compiling with exceptions can be measured (bench averaging or fishtest).

But I don't understand the point of this. Suppose you want to do away with UCI and compile engines as libraries (.so or .dll). With an API to be defined, why would it make sense to throw exceptions inside SF to catch them in the code that calls the library. Wouldn't that make the TBD library protocol dependant on the language (C++) and the compiler used to build the library ?

[ddobbelaere]

Wouldn't that make the TBD library protocol dependant on the language (C++) and the compiler used to build the library ?

That's why people invented the concept ABI, to make this thing work.

EDIT: apparently, like @Sopel97 mentions, this is not recommended as it enforces that all depending programs are compiled the same way (and would exclude languages that don't support exceptions). Catching those exceptions and converting them to error codes in the TBD library API part seems advisable.

[Sopel97]

If this is not a regression I'm definitely for it. Exceptions should be zero-cost on the happy path, and only increase the binary size, which is already large.

But I don't understand the point of this. Suppose you want to do away with UCI and compile engines as libraries (.so or .dll). With an API to be defined, why would it make sense to throw exceptions inside SF to catch them in the code that calls the library. Wouldn't that make the TBD library protocol dependant on the language (C++) and the compiler used to build the library ?

It's possible to do an extern "C" api, which has stable and well defined ABI. I suspect that's what the OP wants to do. Exceptions will allow returning an error code instead of crashing the program that imported the library. Throwing exceptions across ABI boudaries is a big no no

[ppigazzini]

@ab-chesspad you should reschedule the test using "Non-regression STC" bounds
https://tests.stockfishchess.org/tests/view/618078f059e71df00dcc4291

[BM123499]

I've started a test here. I'll delete pending ones for now.

[lucasart]

Should we litter the code with nothrow to make sure that the cost is really zero? At least on the hot path.

[ab-chesspad]

Sorry, but I do not understand what the issue with performance is. May be I missed something, but we are talking about 'exit', the last executable statement in the existing program. Is that what you want to optimize? Regards Alexander Bootman

…

On Wed, Nov 3, 2021 at 5:32 PM lucasart ***@***.***> wrote: Should we litter the code with nothrow to make sure that the cost is really zero? At least on the hot path. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF4K74JQNXRQXFL3AM3WLSTUKHICHANCNFSM5G6CKRVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[BM123499]

@ab-chesspad, unfortunately, your changes fail to pass non-regression test, as shown here: https://tests.stockfishchess.org/tests/view/6181974fe3bcfb9eb3a1aa7b .
So it's believed that this PR is causing a slowdown on SF.

[ab-chesspad]

Please explain how to interpret the test results. Regards Alexander Bootman

…

On Wed, Nov 3, 2021 at 6:15 PM Bruno Melo ***@***.***> wrote: @ab-chesspad <https://github.com/ab-chesspad>, unfortunately, your changes fail to pass non-regression test, as shown here: https://tests.stockfishchess.org/tests/view/6181974fe3bcfb9eb3a1aa7b . So it's believed that this PR is causing a slowdown on SF. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF4K74P72DYSY2FYCSUAYDDUKHNCTANCNFSM5G6CKRVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[BM123499]

@ab-chesspad

after 141520 games, fishtest calculate that this PR loses 0.78 elo. As it doesn't change functionally, it seems it's causing a slowdown.

[ab-chesspad]

I thing I guessed it. The problem is not 'throw exception' itself (I don't believe it is), but -fexceptions flag (correct me if I am wrong).
So, unless we decide to sacrifice .78 loss of ELO, we cannot make SF more robust and user-friendly.

[vdbergh]

Note that the -0.78 elo comes with a CI that includes 0.

IMHO for non-functional changes it is better to do a speed test than a non-regression test, especially if it concerns architectural changes that people like (non-regression tests have a fair chance of failing a neutral patch). If this PR really loses 0.78 elo then this should be detectable by a speed test.

[vdbergh]

From this test #3323 (comment) one can make a guess about the conversion time/UHO_Elo. It seems that 1% speed loss should amount to a loss of approximatively 1 UHO_Elo.

[vondele]

so, this didn't pass CI and is now quite out-dated. I'm somehow sympathetic to the idea of making SF more easily callable as a library, but we can't regress for making that happen. Consequently, I'll close the PR.