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

Implement explicit snapshots #110

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

Implement explicit snapshots #110

wants to merge 1 commit into from

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Dec 27, 2024

See Level/community#118. TLDR:

await db.put('example', 'before')
const snapshot = db.snapshot()
await db.put('example', 'after')
await db.get('example', { snapshot })) // Returns 'before'
await snapshot.close()

This was relatively easy to implement because it's merely exposing an existing LevelDB feature. Which we were already using too (for example, creating an iterator internally creates a snapshot; I now refer to that as an "implicit" snapshot). The only challenge was garbage collection and state management. For that I knew I could copy iterator logic but I wanted to avoid its technical debt, so I first cleaned that up:

  • binding.iterator_close() is now synchronous, because that's 2x faster than napi_create_async_work, let alone executing that async work. Measured with performance.timerify(binding.iterator_close). Simplifies state.
  • I changed the database reference (Database#ref_) from a weak to a strong reference, preventing GC until db.close(). I thought this would remove the need for strong references to iterators & snapshots seeing as the AbstractLevel class already has references in the db[kResources] set, but a new unit test in iterator-gc-test.js proved me wrong. So I kept that as-is (would like to revisit).
  • Having a strong reference to the database removed the need for the IncrementPriorityWork function to increment the refcount of said reference. It now just increments a std::atomic<int> (for our own state outside of V8).
  • Iterators no longer call IncrementPriorityWork which had 2 purposes:
    1. Increase the db reference count. See above.
    2. Delay database close until iterators were closed. A leftover from before abstract-level which closes iterators & snapshots before calling db._close().

I then moved common logic for references and teardown to a new struct called Resource, inherited by Iterator and ExplicitSnapshot. The latter is a new struct that wraps a LevelDB snapshot.

Keeping snapshots open during read operations like db.get() is handled in abstract-level through AbstractSnapshot#ref().

I think further cleanup is possible (and to move more state management to JS) but I'll save that for a future PR.

This PR depends on Level/abstract-level#93 but is otherwise complete.

See Level/community#118. TLDR:

```js
await db.put('example', 'before')
const snapshot = db.snapshot()
await db.put('example', 'after')
await db.get('example', { snapshot })) // Returns 'before'
await snapshot.close()
```

This was relatively easy to implement because it's merely exposing an
existing LevelDB feature. Which we were already using too (for example,
creating an iterator internally creates a snapshot; I now refer to that
as an "implicit" snapshot). The only challenge was garbage collection
and state management. For that I knew I could copy iterator logic but I
wanted to avoid its technical debt, so I first cleaned that up:

- `binding.iterator_close()` is now synchronous, because that's 2x
  faster than `napi_create_async_work`, let alone executing that async
  work. Measured with `performance.timerify(binding.iterator_close)`.
  Simplifies state.
- I changed the database reference (`Database#ref_`) from a weak to a
  strong reference, preventing GC until `db.close()`. I thought this
  would remove the need for strong references to iterators & snapshots
  seeing as the `AbstractLevel` class already has references in the
  `db[kResources]` set, but a new unit test in `iterator-gc-test.js`
  proved me wrong. So I kept that as-is (would like to revisit).
- Having a strong reference to the database removed the need for the
  `IncrementPriorityWork` function to increment the refcount of said
  reference. It now just increments a `std::atomic<int>` (for our own
  state outside of V8).
- Iterators no longer call `IncrementPriorityWork` which had 2
  purposes:
  1. Increase the db reference count. See above.
  2. Delay database close until iterators were closed. A leftover from
     before `abstract-level` which closes iterators & snapshots before
     calling `db._close()`.

I then moved common logic for references and teardown to a new struct
called `Resource`, inherited by `Iterator` and `ExplicitSnapshot`. The
latter is a new struct that wraps a LevelDB snapshot.

Keeping snapshots open during read operations like `db.get()` is
handled in `abstract-level` through `AbstractSnapshot#ref()`.

I think further cleanup is possible (and to move more state management
to JS) but I'll save that for a future PR.

This PR depends on Level/abstract-level#93 but
is otherwise complete.
@vweevers vweevers added enhancement New feature or request semver-minor New features that are backward compatible labels Dec 27, 2024
vweevers added a commit that referenced this pull request Dec 29, 2024
Adds support of two methods:

```js
await db.put('love', 'u')
await db.has('love') // true
await db.hasMany(['love', 'hate']) // [true, false]
```

Depends on a pending `abstract-level` release, and lacks support of
explicit snapshots which should be implemented after #110 lands.

Ref: Level/community#142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor New features that are backward compatible
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

1 participant