Split out JS and pure-C++ components of carmen-cache

[apendleton]

@aarthykc this is the working branch I talked about the other day proposing an alternate way of structuring carmen-cache.

Highlights:

• MemoryCache and RocksDBCache are not NAN/ObjectWrap classes anymore (so, they're not exposed directly to Javascript), they're just pure C++ classes and their functions take normal C++ arguments and return normal C++ types
• There are new classes that wrap these classes and expose them to JS -- they have one member (the underlying C++ type) and basically just translate the data back and forth. Since this logic is mostly the same between the two backends, I also factored them both into a single class that's generic over which backend is being used, so the old RocksDBCache (which used to be a JS type) is now JSCache<RocksDBCache> and the old MemoryCache is now JSCache<MemoryCache> (also aliased as JSRocksDBCache and JSMemoryCache, respectively, though maybe I should just be consistent everywhere).

The goal is to get to the point where rocksdbcache.[ch]pp and memorycache.[ch]pp can be included into other projects that don't know about Node/NAN at all (and don't pull in nan.h) but I'm not there yet.

In scope:

• I've commented out some error handling stuff for now that needs to be put back in -- things that used to throw NAN exceptions need to throw regular C++ exceptions, which need to be caught
• I still need to do coalesce
• Your work in your cleanup branch needs to be merged and continued -- normalization cache, merge (EDIT: killed merge), etc., need to die
• In the process of adding in this layer of abstraction I'm pretty sure I've introduced some extra data copies via pass-by-value arguments or returns -- these are unnecessary and should be cleaned up
• any other new TODO comments that need to be dealt with -- at least one of those keeps tests from fully passing at present
• fix all the warnings, lint errors, etc.

Punting for later:

• In the process of moving JS stuff out of the regular source files I've been moving it back into binding.[ch]pp but this file is starting to get unwieldy again -- probably we need to split it back up again, so, multiple files of JS stuff and multiple files of not-JS stuff
• Expose all this machinery via an extern C wrapper so non-C++ stuff can get at it, and figure out how to build that, maybe alone or maybe including cargo/rustc
• add docs back in -- since before, we had a single function for each thing that was only JS-visible, we wrote JS-centric docs in terms of how we specified what the expected arguments were, etc. Now that things are split, it would make sense to have both JS-oriented docs decorating the JS functions, and C++-oriented docs decorating the pure-C++ functions
• maybe pure-C++ tests (and/or tests of the C interface; if the latter, could be exercised from either C or Rust)

cc @ingalls @springmeyer in case either of you are interested -- this is a spare-cycles experiment, to be clear

[aarthykc]

\o/ this is great!

[ingalls]

This is absolutely fantastic! Going to open the door for so much more work on shared modules!

[springmeyer]

Instead of adding all these pragmas I recommend using -isystem to suppress warnings from headers we don't control like at mapbox/node-cpp-skel#42

[apendleton]

Cool, makes sense. This is just a move of this include from one file to another so I kept it the same, but this is a good tip to apply across the board

[springmeyer]

okay, sorry I missed that - okay then, of course, to handle separately and not in this PR

[springmeyer]

These arguments should likely be passed as const& to avoid copies.

[apendleton]

Yeah for sure, I need to go through and tweak these. I was also looking maybe at using c++17 string_views instead, at least for the strings, because I think I can instantiate them from Rust strings as well in a zero-copy way, whereas I don't think I can make a c++ string from a non-owned bare char* pointer and length without a copy. Will most likely do const& pointers for the vectors, though (looks like there's an array equivalent to string_view, span, but it's in c++20 rather than 17, so probably a no-go).

Do I need to do similar things for functions that return vectors? It's not clear to me if those are copies, or if they'll be automatically optimized into moves, or if I need to manually return std::move(X) or what.

[springmeyer]

Using string_view is a good idea, but no need to use c++17 - you can use protozero::data_view which is a c++11 implementation of string_view.

Do I need to do similar things for functions that return vectors? It's not clear to me if those are copies, or if they'll be automatically optimized into moves, or if I need to manually return std::move(X) or what.

No need to do anything for the return values, the compiler with use RVO without std::move when returning arguments in this case.

[springmeyer]

The goal is to get to the point where rocksdbcache.[ch]pp and memorycache.[ch]pp can be included into other projects that don't know about Node/NAN at all (and don't pull in nan.h) but I'm not there yet.

Great idea. I recommend using https://github.com/mapbox/hpp-skel as scaffolding for pure C/C++ files since it will give you free setup for pure C++ unit tests, sanitizers, and codecov integration, without the complexity that node.js builds add. Yay, no more gyp.

Expose all this machinery via an extern C wrapper so non-C++ stuff can get at it, and figure out how to build that, maybe alone or maybe including cargo/rustc

Is there no way to talk to C++ via rust? If not and you need to wrap in a C API I'm curious how to do this safely. This is not something that I know how to do well, but I did experiment with it once a bunch of years ago: https://github.com/springmeyer/mapnik-c-api

[apendleton]

Is there no way to talk to C++ via rust?

Only via extern C as far as I can tell, judging from simple examples like this -- you can compile them together into a single binary, but Rust can't deal with C++'s name mangling, or complex C++ types (structs, I think, are fine, but not proper classes/objects). I think you end up passing around C++ objects as bare pointers, and exposing C functions that accept those pointers as an argument, cast it to the right type, and then call its respective methods. So yeah, not super safe, and on the Rust side you definitely need to opt into unsafe behavior to do any kind of FFI. It would be something to be very careful about.

[springmeyer]

👌 @apendleton - thanks for the details.

[apendleton]

Progress for today: I went ahead and split up coalesce. Bigger changes here. coalesce is now a synchronous, C++-only function that takes in input as regular arguments and returns results as a vector, so it doesn't know about JS stuff anymore and also doesn't know about threads; it calls coalesceSingle and coalesceMulti directly, and does what coalesceFinalize used to do inline.The objects it takes (PhrasematchSubq objects) also don't wrap JS objects anymore, they wrap the inner cache objects, so a little monkeying with the baton object was necessary to get the reference counting to still work. The JS coalesce function now dispatches to a single function coalesceTask that runs in the threadpool, pulls the contents out of the baton, sends them to the sync function, and moves their return value back into the baton again.

I also got clang tidy and clang format to pass. It looks like maybe a new version of clang tidy got introduced into Mason or something, because a bunch of stuff changed, and the config file needed tweaking to continue to behave the same. @aarthykc FYI: clang-tidy decided all on its own to change a couple of the std::string params to const& std::string, which I know you're also working on. I didn't manually change any though, so the others should still be there for you to take care of.

[apendleton]

This has been stagnating for awhile and is preventing other carmen-cache work because of concerns about branch divergence, so I'm wrapping this up and pruning things from the scope. The main PR body is up to date, but to be explicit, what's in:

• separate all the C++ stuff from all the JS stuff
• eliminate the normalization cache

what's not, for now:

• actually make a mechanism for build just the C++ parts
• write C++-only tests
• expose a C interface for access from Rust
• switch from string references and vector references to slice-like things (string-views, spans, etc.)

These things can all be done non-disruptively later, and won't interfere with unrelated work that needs to happen in carmen-cache. Things I'm doing now:

• run carmen tests against this carmen-cache
• compare benchmarks between this carmen-cache and master to ensure no significant speed regressions

[apendleton]

At least running locally, this looks like a ~1% performance diff between this branch and master, which might well just be noise.

[apendleton]

@springmeyer nobody else on Search besides me felt comfortable reviewing this PR -- if you have a sec, would you mind giving it another glance so we can go ahead and merge it? The two things you had suggested before were the pragma change, which we had decided was out of scope for this PR, and the const& change -- that one's done (and we just went with plain const references for now -- I'm still interested in string_view/span but punting for the moment).

The main additional changes that have taken place since you last reviewed besides that pass-by-reference stuff:

• gave coalesce and friends the same break-up treatment, so they're now also stand-alone
• deleted a bunch more stuff we're not using anymore (thanks @aarthykc )
• added a few tests to improve coverage

We also aren't adding stand-alone tests or a non-JS Makefile right now; again, definitely still interested, but mostly just looking to get this merged sooner rather than later so that branch divergence doesn't keep other unrelated streams of work from starting.

[springmeyer]

This looks good. I did a quick scan through all the files. Obviously a lot of changes, but everything looks reasonable on first glance. The new use of auto and other refactorings seem 👍