josephg / node-foundationdb

Modern Node.js FoundationDB bindings
Other
115 stars 17 forks source link

FDB api should be called off main thread #46

Open ex3ndr opened 4 years ago

ex3ndr commented 4 years ago

We have very small test:

let buffer = Buffer.alloc(0);
await db.doTn(async (tx) => {
    for (let i = 0; i < 10000; i++) {
        let id = uuid();
        tx.set(id, buffer);
    }
})

We found that with this code js thread is blocked by FDB for ~160ms (of 180ms overal).

Further investigation shows that the issue is within FDB C library.

We changed code to use versionstampkeys

let buffer = Buffer.alloc(0);
await db.doTn(async (tx) => {
    for (let i = 0; i < 10000; i++) {
        tx.setVersionstampSuffixedKey(buffer, buffer);
    }
})

Blocked time inside of a transaction went down to just 30ms, but commit time was increased by 150ms. I suppose that FDB library is offloading some computation to the end for atomic operations and just blocks thread later.

Isn't we should avoid blocking event loop and have a separate thread and schedule all FDB operations there?

josephg commented 4 years ago

That difference is strange; and much slower than I’d expect. Can you share the profile trace?

ex3ndr commented 4 years ago

What trace do you want? NodeJS? Low level flamegraph? Client one?

I have tried to do read client traces and found transaction warnings about it's size. I have added TransactionOptionCode.NextWriteNoWriteConflictRange and all warnings from client trace are missing now and total time reduced down to versionstamp times.

Also replacing of a text uuid with a binary one gave 30% speedup. It is clearly related to a key/value size. I yet to find any code in NAPI or in your bindings that rely on the size of the data, but FDB have a lot of logic that depends on the size of data.

In trace logs such transaction is about 4MB size (while k/v size is 10 times less), some of the overhead came from write conflicts, some from data itself, some for internal library stuff. It is obvious for me that having huge hash set (for write conflicts), huge map for actual values and other utility data in a library would cost time and block main thread for some time.

josephg commented 4 years ago

Any - I'm curious to see where the time is going. I can probably do a profiling trace myself if its too tricky to share.

And yeah that does make sense; presumably most of that time is going into checking if one of the writes overrides a previous write in the same transaction. Given there's a known maximum size for each transaction, a statically allocated btree or something should be 3-4 orders of magnitude faster than the performance you're seeing. Might be worth filing an issue upstream on foundationdb to fix the performance of the C fdb client library. Or opening a conversation in the forum about it.

I could move native calls to a separate thread, but thats some complexity that shouldn't be necessary. There's no reason for the client library to be so slow in this instance.

ex3ndr commented 4 years ago

Created a new post on the forum: https://forums.foundationdb.org/t/bindings-seems-to-be-very-slow-for-a-simple-operation/2275.

ex3ndr commented 3 years ago

Well, it seems that this happens for every bindings. I think all other languages are a little bit different from node. All other languages somehow ok with this, but it is not ok for a node.

All this is not related to how NodeJS works and simply blocking main thread is not an option since it is the only thread on JS.

It seems it might be a good thing to free main thread from possibly unpredictable FDB bindings.

josephg commented 3 years ago

I agree it should be fast but doing 10k tiny writes within a single transaction is an unusual way to use fdb. Like, this isn’t something all users will run into.

In terms of implementation, we’d need to:

If we have one thread for all transaction messages, total throughput won’t change much in test scripts like yours; because the bottleneck would just move to the offload thread.

It’s doable. But again, all tnx.set does in the C client code is add the data to a list to be sent on commit. And marks that key so it gets retrieved if you subsequently call get() later in the txn. If that code was 100x faster, there would be no need to offload calls onto another thread. And a 100x speedup sounds pretty doable there. And that fix would improve performance for users in other languages too.

Anyway, if you’re convinced your proposed solution is the right way forward, put together a patch. If it’s clean enough and doesn’t break the binding tester I’ll merge it.