Possible huge write performance boost

[quolpr]

I was discovering why absurd-sql is very fast even on large datasets write, and why wa-sqlite doesn't have such performance (when table has 100,000+ records). And I discovered something interesting — absurd-sql makes write only on transaction finish. During the transaction, it makes reads from IDB, but writes are happening in-memory. Then it dumps all the data to IDB on transaction finish https://github.com/jlongster/absurd-sql/blob/master/src/sqlite-file.js#L374

[quolpr]

When I make insert of a lot of data, I am getting a lot of reeds and the write requests:

If we could postpone the writing to the final step(on transaction commit), then it should improve performance. I am using IDBBatchAtomicVFS

[quolpr]

Actually, I tried to add some cache on data write during transaction, and if the block will be read again from the same offset — it will have a cache hit. I also reset cache on SQLITE_FCNTL_SYNC. The write performance improved dramatically 🤔

[rhashimoto]

Hmm. I'm not sure why buffering the writes should speed things up, provided that wa-sqlite is using a single IDB transaction for a SQLite transaction (you can log IDB transactions by uncommenting this line to check if it is succeeding in minimizing transactions). The writes need to go to IDB eventually, so I don't see an obvious advantage in deferring them. The only thing I can think of is that if a lot of blocks are written more than once in the transaction then overwrite in memory might be a lot faster than overwrite in IDB, but I wouldn't be certain of that because I would hope that expense would be mostly off-thread.

If a lot of blocks are being read after being written, then yes, that's going to be a performance hit. I would see whether increasing the SQLite cache size helps to avoid that. I would give that a try before inserting another cache at the VFS level.

[quolpr]

If a lot of blocks are being read after being written, then yes, that's going to be a performance hit. I would see whether increasing the SQLite cache size helps to avoid that. I would give that a try before inserting another cache at the VFS level.

I made block size 32kb and cache_size to -5000, but it is still suffering if whole database can't be stored at memmory.

I made this for xRead:

      const fromCache = this.cache.get(-iOffset);
      if (fromCache && fromCache.length === pData.size) {
        pData.value.set(fromCache);

        return VFS.SQLITE_OK;
      }

And this for xWrite:

      if (iOffset !== 0) {
        this.cache.set(-iOffset, block.data);
      }

You can check this commit — kikko-land/kikko@e1da525

The write performance now even better than absurd-sql at some cases 🤔

[rhashimoto]

If I'm reading your code correctly, you don't limit your cache size. I'd be interested in how big it gets. Then compare the performance with an unchanged VFS with cache size increased to that number plus 2000 (assuming that's more than the 5000 you already tried). I think that would be closer to apples-to-apples memory usage. It's still possible your modified VFS would be faster since the SQLite cache contains both read and written blocks.

I had caches in some earlier VFS classes, but eventually I did away with them. I didn't like that blocks were duplicated if the SQLite cache was active, and disabling the SQLite cache isn't advisable because it is the fastest. I also capped my cache size because I wanted to allow very large transactions, but then I had to tune that cache size and the SQLite cache size, plus it was more code and tracking down cache bugs is not a lot of fun. So now I think really hard about whether I want to implement another cache and so far the answer has been no. Just something to consider even if there is a performance win.

I guess it shouldn't be surprising that IDBBatchAtomicVFS can be competitive with absurd-sql on pure write performance (though I wasn't expecting that). IDBBatchAtomicVFS.xWrite is synchronous (unlike xRead), so the Asyncify overhead should be small, i.e. it doesn't have to do any unwinding or rewinding.

[quolpr]

Yep, I don't limit. That is downside, and memory problems could be here.

Also, thank you so much for detailed response! It helps me a lot 🙂 And thank you for the all the work you did, it's an excellent job.

[quolpr]

Some update: actually increasing cache size to -20,000 did the job — the write performance is the same as with the cache. You were right!

[quolpr]

And some other update: but actually, after some more inserts, the write performance begin to degrade (the DB become larger). While with cache — it remains constant, or degrading not so fast 🤔

[quolpr]

@rhashimoto finally I was able to achieve the same write performance as absurd-sql has. I am not sure why, but your IDBContext/Store has a bottleneck somewhere. So instead of:

await db.run("readwrite", async ({ blocks }) => {
  await Promise.all(
    Array.from(this.blockToWrite.keys()).map(async (k) => {
      const toWrite = toWriteBlocks.get(k)!;

      await blocks.put(toWrite);
    })
  );
});

I put:

await new Promise<void>((resolve, reject) => {
  // @ts-expect-error lib dom misses third argument
  const tx = idb.transaction("blocks", "readwrite", {
    durability: "relaxed",
  });

  const blocksStore = tx.objectStore("blocks");

  for (const k of toWriteBlocks.keys()) {
    const toWrite = toWriteBlocks.get(k)!;

    blocksStore.put(toWrite);
  }

  tx.addEventListener("complete", () => {
    resolve();
  });
  tx.addEventListener("abort", reject);
  tx.addEventListener("error", reject);
});

And this code run double faster than the first one 🤔

[quolpr]

If you do want to allocate times to particular things then you should be doing it in a much less invasive way (I tried to show you such a way).

Who says which way should I do the things?

And even if invasive means were the only way to do the measurement (which is not the case here) why would you want to commit that to production code?

I open to any discussion, why do you think that I want to commit strait forward to the production code? I agree, that I should put Promise.all to the PR, that is fair point. But why do you say, that I force pushing it to the production code?

If you main that thing, that I made a fork of absurd-sql — you still have the choice to not use it. I don't push anybody to use my fork or to agree with my discussions.

And yes, I don't mind that the queueing of IDB operations is done off-thread instead of in my code. In fact, that's exactly where I want it to be. When IndexedDB throughput is my critical path, I want to keep that pipeline full as much as I can, and that means not going back to Javascript for every operation.

Why do you think, that the author absurd-sql did the same thing as you? The absurd-sql code part looks more like a mistake, cause in method declaration async is present, but no await in method body.

And if it was conceived, why the author didn't mention it in the article? Like, hey, write operations are faster because they are actually delayed. And to make fair comparison, you should actually do write+read benchmark in the same row.

[quolpr]

Your attempt with Promise.all() is still bad, but slightly better:

But why if I put Promise.all on your benchmark it shows the same number, as without awaiting? https://jsfiddle.net/adsoyqr1/3/

And it is not slightly, it actually the same

[rhashimoto]

This is your pull request to the master branch of absurd-sql, right? That's what I'm talking about. It will make absurd-sql slower overall. I don't see the point of comparing with a baseline that you have made slower, which is why I asked what you were comparing with.

It looked like you thought you were also fixing a bug, and if that was a valid fix then you would be perfectly justified in comparing with the fix added. And so I tried to show you that your change could not fix that database corruption bug.

I'm not telling you what to do in your own tree. I'm telling you that your PR is flawed, which I think you now realize, and using a version of absurd-sql with that change in it to make comparisons (which I don't know if you were doing or not, hence my question) would not be valid.

[rhashimoto]

But why if I put Promise.all on your benchmark it shows the same number, as without awaiting?

Yes, I said this was better, and you're correct that the microbenchmark shows no time penalty. But I still don't think this code is good. The reason the await doesn't hurt here is because there is nothing left to go into the IndexedDB pipeline before the transaction commits and everything goes away. Here's why I think this is still the wrong way to go:

1. In a real application you could still be blocking new operations from going into the IndexedDB pipeline. If the next thing the application does is open a new transaction and start creating requests on it, which is actually quite likely, await will delay that from happening. You have cleaned up the pipeline bubbles between operations but you're still risking bubbles between transactions that harm overall performance.

2. If performance is important to you, in general you need a pretty good reason (e.g. correctness) to block the critical path. Using benchmarking as one of those reasons seems so counter-productive. Code smell is a subjective criticism, I admit, but in my opinion it applies here (and that isn't meant pejoratively, or to deny the stench in much of my own code).

3. There are workable alternatives. You can use the same IndexedDB event hooks to monitor progress, just without using await on the critical path.

[quolpr]

jlongster/absurd-sql#56 to the master branch of absurd-sql, right? That's what I'm talking about. It will make absurd-sql slower overall. I don't see the point of comparing with a baseline that you have made slower, which is why I asked what you were comparing with.

I am comparing just the real write performance, nothing more.

It looked like you thought you were also fixing a bug, and if that was a valid fix then you would be perfectly justified in comparing with the fix added. And so I tried to show you that your change could not fix that database corruption bug.

Yes, you are right. I was wrong. But still, the inserts at absurd-sql are not measured right.

In a real application you could still be blocking new operations from going into the IndexedDB pipeline. If the next thing the application does is open a new transaction and start creating requests on it, which is actually quite likely, await will delay that from happening. You have cleaned up the pipeline bubbles between operations but you're still risking bubbles between transactions that harm overall performance.

I agree for wa-sqlite, but not for absurd-sql case. When I put Promise.all on write, the next query still will be in IDB transaction, cause transaction almost never complete because worker uses Atomic awaiting (so the next call in event loop still will be an IDB call).

If performance is important to you, in general you need a pretty good reason (e.g. correctness) to block the critical path. Using benchmarking as one of those reasons seems so counter-productive. Code smell is a subjective criticism, I admit, but in my opinion it applies here (and that isn't meant pejoratively, or to deny the stench in much of my own code).

I didn't say anything about code smell. And I didn't say that we should fix this cause of code smell. I just point, that James could just miss the await phase of bulkWrite