Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CHG: Add kryszyp lmdb-js improvements #205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Llorx
Copy link

@Llorx Llorx commented Apr 6, 2022

Just for testing, I just copied lmdb-js changes to node-lmdb and works like a charm (at least the mapsize autogrow without needing to relaunch the transaction).

Haven't done any further tests, but looks like it should work out of the box.

EDIT: Just noticed a weird problem. Sometimes it fails with 0xC0000005‬ after a couple of auto-regrows and trying to do a putString. This is the test:

import FS from "fs";
import PATH from "path";

import LMDB from "node-lmdb";

const DATA_PATH = PATH.join(__dirname, "data");
try {
    FS.mkdirSync(DATA_PATH);
} catch (e) {}

export class DBTest {
    env = new LMDB.Env();
    dbi:LMDB.Dbi = null as any;
    dbi2:LMDB.Dbi = null as any;
    async init() {
        try {
            FS.rmSync(PATH.join(DATA_PATH, "test"), { recursive: true });
        } catch (e) {}
        try {
            FS.mkdirSync(PATH.join(DATA_PATH, "test"));
        } catch (e) {}
        this.env.open({
            path: PATH.join(DATA_PATH, "test"),
            mapSize: 16*1024,
            maxDbs: 16
        });
        this.dbi = this.env.openDbi({
            name: "cosas",
            create: true
        });
        this.dbi2 = this.env.openDbi({
            name: "cosas2",
            create: true
        });
    }
    async run() {
        this.performance();
        this.canRollback();
        this.sequential();
    }
    performance() {
        this.logger.debug("Performance test");
        const ITEM_COUNT = 100000;
        //@ts-ignore // Why no typings?
        let now = performance.now();
        let tx = this.env.beginTxn();
        for (let i = 0; i < ITEM_COUNT; i++) {
            tx.putString(this.dbi, "testtesttesttesttesttesttesttesttesttesttest" + i, "testtesttesttesttesttesttesttesttesttesttesttest");
        }
        tx.commit();
        //@ts-ignore // Why no typings?
        let dif = performance.now() - now;
        this.logger.debug(`${ITEM_COUNT} DB put performance:`, Number(dif.toFixed(3)), "ms");
        //@ts-ignore // Why no typings?
        now = performance.now();
        let count = 0;

        tx = this.env.beginTxn();
        let cursor = new LMDB.Cursor(tx, this.dbi);
        for (let item = cursor.goToFirst(); item !== null; item = cursor.goToNext()) {
            count++;
        }
        tx.commit();
        this.assert("All items received", count, ITEM_COUNT);
        //@ts-ignore // Why no typings?
        dif = performance.now() - now;
        this.logger.debug(`${ITEM_COUNT} DB iteration performance:`, Number(dif.toFixed(3)), "ms");
    }
    canRollback() {
        this.logger.debug("Rollback test");
        try {
            let tx = this.env.beginTxn();
            for (let i = 0; i < 10000; i++) {
                tx.putString(this.dbi, "test" + i, "test");
            }
            tx.abort();
        } catch (e) {}
        let tx = this.env.beginTxn();
        this.assert("Abort DB transaction", tx.getString(this.dbi, "test5000"), null);
        tx.commit();
    }
    sequential() {
        this.logger.debug("Sequential test");
        let tx = this.env.beginTxn();
        for (let i = 0; i < 10000; i++) {
            tx.putString(this.dbi, "asd" + i, "test");
        }
        tx.commit();
        tx = this.env.beginTxn();
        this.assert("Sequential DB transactions", tx.getString(this.dbi, "asd5000"), "test");
        tx.commit();
    }
    async dispose() {
        this.dbi.close();
        this.dbi2.close();
        this.env.close();
        try {
            FS.rmSync(PATH.join(DATA_PATH, "test"), { recursive: true });
        } catch (e) {}
    }
}

Fails on canRollback test.

Is a pretty straightforward test (instantiates the class, runs awat init(), then runs await run() and then runs await dispose()).

Keeping the PR here just in case @kriszyp can throw a bit of light.

@Llorx
Copy link
Author

Llorx commented Apr 6, 2022

My implication is that I want kryszyp features, but with a sync transaction that also can be aborted. For this, lmdb-js forces me to create a childTransaction which runs on a separate thread and I don't want this right now.

@kriszyp
Copy link
Collaborator

kriszyp commented Apr 6, 2022

I'm a little hesitant to try to port over large chunks of these partial changes from lmdb-js, since the whole point of making this separate lmdb-js is to house these more significant changes, and having partial ports seems like a lot of additional testing effort.

My implication is that I want kryszyp features, but with a sync transaction that also can be aborted.

Why isn't this working for you in lmdb-js, it has abortable sync transactions?

import { ABORT } from 'lmdb';
db.transactionSync(() => {
  db.put(k, v);
  return ABORT; // undo last put, abort the txn
});

@Llorx
Copy link
Author

Llorx commented Apr 6, 2022

Ok, the problem I had was just that I forgot to close a cursor 🤦‍♂️

Further tests show that it works out of the box, yes.

Why isn't this working for you in lmdb-js, it has abortable sync transactions?

And I'm short-sighted and was just reading for rollbacks, which childTransaction is the only one that has references to that. I've should read about transactionSync and ABORT. Going to do it that way instead. Thank you!

@Llorx
Copy link
Author

Llorx commented Apr 6, 2022

@kriszyp check this:

import LMDB, { ABORT } from "lmdb";
console.log(LMDB.ABORT); // undefined
console.log(ABORT); // OK

Just for you to know, if that's something about lmdb-js or maybe is just about TypeScript and the conversion I have on my side. The typings says that LMDB.ABORT should actually work (although it says that the value is 10000000000 instead of {} xD)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants