Entering an empty array as a key throws an error

[kylebernhardy]

We need to track all non-null/non-undefined/non-empty string values in our indicies. One of these scenarios can be an empty array. In previous versions we could store an empty array as a key. In 2.1.6 it throws an the error: Error: Invalid key or zero length key is not allowed in LMDB

Sample code:

'use strict';

const lmdb = require('lmdb');

let env_init = {
    "path": './data',
    sharedStructuresKey: Symbol.for('structures'),
    overlappingSync: true
};
let env = lmdb.open(env_init);

let dbi = env.openDB('keywords', {
    create: true,
    dupSort: false,
    useVersions: false,
    sharedStructuresKey: Symbol.for('structures')
});

dbi.putSync([], 1);

[kriszyp]

Does this... work? When I run your code with v1.5.5 and then call dbi.get([]) it returns undefined. I think the put is just silently failing in older lmdb-store (zero-length keys really aren't allowed), so throwing an error actually seems greatly preferable.

I suppose we could have some binary representation of []. However, arrays as keys are treated as just a sequence of keys, so for example ['hi'] and 'hi' are identical binary representations, so zero bytes is the most intuitive representation of [] (which indeed is an illegal key in LMDB). We could serialize it as a null (one byte of 0), I suppose, but it would deserialize to null.

[kriszyp]

BTW, as an aside, I'm curious if you have had any chance to run your benchmarks testing the new overlappingSync flag. I was hopeful that it might provide some performance boost, but don't know if that actually is measurable in your tests.

[kylebernhardy]

Ah, I see. we had never verified in our tests that the empty array actually committed. Thank you for pointing that out & I agree an error is much more preferred. We can handle the empty array on our end, thank you for your feedback on this.

My goal today / tomorrow is to run our benchmarks on the overlappingSync flag. Once I have all of our tests passing that is my next step. Just to check is that flag crash proof? If there is a hard crash will the environment stay in a non-corrupted state? I assume yes as noSync defaults to false.

[kriszyp]

Just to check is that flag crash proof? If there is a hard crash will the environment stay in a non-corrupted state?

Yes, that is the one of the primary reasons/goals of the overlappingSync mode. I put together a little more detailed explainer of this here: https://github.com/DoctorEvidence/lmdb-js/discussions/131

[kylebernhardy]

I ran a quick test locally and with noSync: false / overlappingSync: false. I get ~2300 txns / sec (Each transaction writes to 10 dbis) with noSync: false / overlappingSync: true I get ~ 2600 txns / sec which is about a 15% boost. Would this be about what you would expect to see?

[kriszyp]

Yes, that is about the same level of performance boost that I measured (on a much slower computer :) ), and what I was hoping you would get 👍. I think this pretty much driving the I/O as continuously and hard as possible.

I would think that in situations where the async txn callbacks have to do more CPU work, you might see a little more perf benefit from overlapping sync just because this would enable a little more "even" concurrency between CPU (during transaction) and I/O (during flush). But hard to predict that stuff.

[kylebernhardy]

It's very cool work you have put into this. Getting more gains while maintaining durability is a huge challenge. Thank you again for all of the work you have done.

[kriszyp]

Actually, you should try the benchmarks again with v2.2.0, I found that using a more asynchronous method of flushing/syncing seemed to produce a lot better performance for me.

[kylebernhardy]

I'm excited to benchmark it. I'll update to 2.2 tomorrow & will let you know the performance boost I see.

[kylebernhardy]

@kriszyp you are a wizard. My tests were all local with a Hyper-V VM Ubuntu 20 with 4 cores. My tests were run via k6 with 200 VUs running for 60s per run. The writes were all 10 attribute objects & we are fully indexing each entry. All test were the same codebase, just with different version of LMDB and environment settings. Your new changes are getting very close in approaching nosync = true, while being crash proof! If you need benchmarks in the future let me know & I will be happy to run them.

2.1.7, noSync = false: Run 1 - 1890 txns / s Run 2 - 1940 txns / s Run3 - 1900 txns / s

2.1.7, overlappingSync = true: Run 1 - 2060 txns / s Run 2 - 2166 txns / s Run3 - 2174 txns / s

2.1.7, noSync = true: Run 1 - 5460 txns / s Run 2 - 4640 txns / s Run3 - 5646 txns / s

2.2.0, noSync = false: Run 1 - 4141 txns / s Run 2 - 3686 txns / s Run3 - 3798 txns / s

2.2.0, overlappingSync = true: Run 1 - 4166 txns / s Run 2 - 4119 txns / s Run3 - 3788 txns / s

[kriszyp]

That looks great! A couple things to note just to make sure we are being fair. With the latest async calls to sync/flush the data, the syncing operations, and outstanding dirty pages can actually get a lot more backed up. In your tests, it would probably be good to try a run with await db.flushed at the end of the benchmark prior to computing the txn/sec, if you want to do an equal comparison for durable writes (although the speed of "visible" write txns is certainly a worthwhile and pertinent on its own).

Also, just note that overlappingSync defaults to true on 2.2.0 (partly thanks to your verification efforts :) ), so you would need to explicitly turn it off if you want it disabled, (so I think your last two tests on v2.2 were identical). Setting noSync=true should also disable overlappingSync.

All that said, even if there some dirty/write backup occurring, this seems like a great performance boost in typical transaction interactions. I measured a huge improvement on an old slow mechanical HD, but great to see this prove out on a modern/fast SSD, thanks for testing this.

[kylebernhardy]

I'll see if I can add some extra logging to track the flush aspect of the txn so we can get a full understanding of write latency. Also what I saw was higher Mb/s write to disk with fewer transfers / s. I did not try to scale up my VUs to see if write throughput increases with higher pressure, that will happen in the future. Again, if you need any testing on our end feel free to reach out. My goal is to setup a github action so we can easily spawn load tests.

[kriszyp]

Also what I saw was higher Mb/s write to disk with fewer transfers / s

Nice, I think that's exactly what we want, and how we would hope that the OS (and disk) would optimize bigger queues of writes!