SWI-Prolog / packages-cpp

The SWI-Prolog C++ interface
30 stars 14 forks source link

Clean-up and some simplification #19

Closed kamahen closed 2 years ago

kamahen commented 2 years ago
  Support more compilers (g++,clang): 32-bit, no-GMP, Windows, MacOS
  PlTerm methods check for exception on failure and throw appropriately
  Rename PlTerm::unify_*_ex() methods to unify_*_check()
  Rename PlTerm::get_*() methods to as_*()
  Improve error handling (and documentation for error handling)
  Handle std::bad_alloc
  Add convenience class PlForeignContextPtr<ContextType> for non-deterministic predicates
kamahen commented 2 years ago

This is an early version of improvements to SWI-cpp2.h, so that it can be tested on platforms I don't have, such as MacOS. I am going to proof-read this; and I hope that other people will review the code and documentation. (The documentation is still a bit rough, and shows my limitations as a technical writer.)

JanWielemaker commented 2 years ago

Bad luck on Apple-Clang :cry: See attachment. I see you out the trick with the 4 unify_integer things back. If I recall well, this problem was gone if we called the explicit versions?

apple-clang.txt

I start to like mapping logical failures into exceptions. I do think we should make a decision though and either do it using a return value or using exceptions. The latter seems attractive. Do you have figures on the performance impact?

kamahen commented 2 years ago

@JanWielemaker - did you intend to merge this PR even though it doesn't compile on MacOS?

On MacOS, is it the case that sizeof (int) == 8? (And what's sizeof (long)?)

(I'll make some benchmarks for throw PlFail() vs return false, and also for the C vs C++ interfaces. There should be no overhead for the try if there's no throw, according to the g++ docs)

(The changes I've made to rocksdb take advantage of the overloading methods for int, so I'll need to check them and also add some more test cases)

JanWielemaker commented 2 years ago

did you intend to merge this PR even though it doesn't compile on MacOS?

I merged into master, but the main repo points at the last working version. I messed up a bit last time by merging this before testing all platforms, not expecting any serious problems. We'll just continue until it compiles on all platforms and we are happy with the interface. Then I'll see what to do.

MacOS is a normal POSIX 64 bit environment, e.g. int=32, long=64 and ptr=64.

That exceptions are a bit slow is not too much of an issue. The PlFail may happen a in more time critical situations, though probably it is not too bad. Failing unifications in foreign code are also fairly rare. Note that you cannot even do e.g. if ( !PL_unify(t1,t2) ) PL_unify(t1,t3) as even if the first unification fails it may already have created (trailed) bindings. So, you first have to backtrack before you can try anything else. As an escape you can use PL_open_foreign_frame() which allows discarding and rewinding the frame, doing the backtracking. These oddities make the exception route attractive.

kamahen commented 2 years ago

Using PlThrow() instead of return false results in a 15x or 20x slowdown (depending on how much inlining the compiler does). The overhead of C++ vs C is negligible (the compiler does a good job of inlining) g++ 12.2.0

So, I'll add a remark about performance to the documents.

I'll also need to add some comments about PL_open_foreign_frame() for unification failure (and will review what's written in foreign.doc)

kamahen commented 2 years ago

For figuring out what's going on with apple-clang, please run the attached programs and give me the output (they should be identical; however, I couldn't get any output when I ran the C++ program with mingw/wine. (Please specify -Wall in case there's something wrong with the code)

I've also copied these to /home/peter/src/swipl-devel/packages/cpp/sizes.c* on dev.swi-prolog.org

I was a bit surprised by the mingw results (I've put them in comments in sizes.c) ... it's possible that people who write foreign predicates and don't specify int64_t will get unexpected results.

sizes.zip

JanWielemaker commented 2 years ago

All is pretty standard:

clang -Wall sizes.c && ./a.out   

INT_MAX:    4 7fffffff 2147483647
UINT_MAX:   4 ffffffff 4294967295
LONG_MAX:   8 7fffffffffffffff 9223372036854775807
ULONG_MAX:  8 ffffffffffffffff 18446744073709551615
LLONG_MAX:  8 7fffffffffffffff 9223372036854775807
ULLONG_MAX: 8 ffffffffffffffff 18446744073709551615
SIZE_MAX:   8 ffffffffffffffff 18446744073709551615

clang++ -Wall sizes.cpp && ./a.out

INT_MAX:    47fffffff  2147483647
UINT_MAX:   4ffffffff  4294967295
LONG_MAX:   87fffffffffffffff  9223372036854775807
ULONG_MAX:  8ffffffffffffffff  18446744073709551615
LLONG_MAX:  87fffffffffffffff  9223372036854775807
ULLONG_MAX: 8ffffffffffffffff  18446744073709551615
SIZE_MAX:   8ffffffffffffffff  18446744073709551615

clang --version
Apple clang version 13.1.6 (clang-1316.0.21.2.5)
Target: arm64-apple-darwin21.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

I don't know the relation between Apple clang and clang as open source compiler. It could well be that this is an "Apple" surprise :cry:

JanWielemaker commented 2 years ago

Using PlThrow() instead of return false results in a 15x or 20x slowdown (depending on how much inlining the compiler does).

I'm still rather unhappy with the two different versions of the unify methods. I think it is overkill and more confusing than it needs to be. Such a slowdown is a bit of a bummer. Yes, it won't happen too often, but it is also not that uncommon to have a predicate where unification failure is rather normal. This result causes me to have the unification methods (only) as booleans with a [[nodiscard]] property.

I'll also need to add some comments about PL_open_foreign_frame() for unification failure (and will review what's written in foreign.doc)

It is fairly extensively documented with PL_unify(). Of course, improving the text is always welcome. This does not describe the interaction with attributes: there is no wakeup on attributes in foreign code. The wakeup of triggered constraints happen after the foreign predicate completes. That also applies for the atomic unifications. Otherwise, the story of partial unifications does not apply when unifying with atomic data as these create only a single binding. Possibilities for overflows due to lack of trail space or global stack space for triggering a wakeup are validated before we begin, so there are no partial results. This unlike PL_unify() as that may result in any number of bindings and goals scheduled to wakeup.

kamahen commented 2 years ago

Maybe these files will have a clue in them?

./build/CMakeFiles/3.18.4/CMakeCCompiler.cmake
./build/CMakeFiles/3.18.4/CMakeCXXCompiler.cmake

Also, there's a build.ninja file, which has lines in it like:

build packages/cpp/CMakeFiles/plugin_cpp4pl.dir/cpp4pl.cpp.o: CXX_COMPILER__plugin_cpp4pl_RelWithDebInfo ../packages/cpp/cpp4pl.cpp || cmake_object_order_depends_target_plugin_cpp4pl
  DEFINES = -Dplugin_cpp4pl_EXPORTS
  DEP_FILE = packages/cpp/CMakeFiles/plugin_cpp4pl.dir/cpp4pl.cpp.o.d
  FLAGS = -O2 -g -DNDEBUG -fPIC -Wall -D__SWI_PROLOG__ -std=gnu++17
  INCLUDES = -Ipackages/cpp -I../src/os -I../src
  OBJECT_DIR = packages/cpp/CMakeFiles/plugin_cpp4pl.dir
  OBJECT_FILE_DIR = packages/cpp/CMakeFiles/plugin_cpp4pl.dir
  TARGET_COMPILE_PDB = packages/cpp/CMakeFiles/plugin_cpp4pl.dir/
  TARGET_PDB = packages/cpp/cpp4pl.pdb
kamahen commented 2 years ago

I suppose that requiring the programmer to write PlCheck(A1.unify_term(A2)) instead of A1.unify_term_check(A2) isn't unreasonable (it turns out to be about 30% slower on failure, but if that matters, the programmer should probably use the return false style).

At Google, the code tended to be littered with CHECK(some_call(...)) for stuff that was supposed to always succeed. This made the code a bit harder to read, but I got used to it (the important thing being the some_call(...), not the CHECK).

JanWielemaker commented 2 years ago

I would be happy with the Plcheck() approach, leaving the boolean as normal recommended way to deal with logical failure. In the xpce sources we used TRY(...). Doesn't work so well in C as there is no automatic cleanup.