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.

Sign up for GitHub

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

Jump to bottom

Implement explicit snapshots #93

Merged
merged 1 commit into from
Dec 27, 2024
Merged

Implement explicit snapshots #93

merged 1 commit into from
Dec 27, 2024

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Dec 22, 2024 •
edited
Loading

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()

@vweevers vweevers mentioned this pull request Dec 22, 2024
Open
11 tasks
vweevers added a commit to Level/memory-level that referenced this pull request Dec 22, 2024
@vweevers
Implement explicit snapshots
2bb79fa 
See Level/community#118. Depends on
Level/abstract-level#93.

Category: addition
@vweevers vweevers mentioned this pull request Dec 22, 2024
Draft
@vweevers vweevers added the semver-minor New features that are backward compatible label Dec 22, 2024
@vweevers
Copy link
Member Author

vweevers commented Dec 22, 2024 •
edited
Loading

Remaining issues to solve or postpone:

  1. Does not support deferred open, meaning db.open() is required before db.snapshot(). I'm okay with that.
  2. Sublevel behavior is not symmetric. Normally a db.close() closes the snapshot too but this isn't the case on sublevel.close() because the snapshot belongs to the parent database (even if created via sublevel.snapshot()).

@vweevers
Copy link
Member Author

Sublevel behavior is not symmetric.

Fixed.

@vweevers vweevers marked this pull request as ready for review December 23, 2024 18:53
vweevers added a commit to Level/memory-level that referenced this pull request Dec 23, 2024
@vweevers
Implement explicit snapshots
22e9e64 
See Level/community#118. Depends on
Level/abstract-level#93.

Category: addition
vweevers added a commit to Level/classic-level that referenced this pull request Dec 27, 2024
@vweevers
Implement explicit snapshots
c2a6c3d 
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 mentioned this pull request Dec 27, 2024
Open
@vweevers vweevers merged commit f81d348 into main Dec 27, 2024
11 checks passed
@vweevers vweevers deleted the explicit-snapshots branch December 27, 2024 13:41
vweevers added a commit that referenced this pull request Dec 29, 2024
@vweevers
Improve snapshot documentation
725e313 
- Add `snapshot` to TypeScript option types
- Remove redundant documentation from README
- Don't mark explicit snapshots as experimental. I'm confident in this
  feature now (having finished the implementation in `classic-level`)
  and I'm happy with the API.

Ref: Level/community#118
Follow-Up-To: #93
@vweevers vweevers mentioned this pull request Dec 29, 2024
Merged
vweevers added a commit that referenced this pull request Dec 29, 2024
@vweevers
Improve snapshot documentation (#98)
b5b583c 
- Add `snapshot` to TypeScript option types
- Remove redundant documentation from README
- Don't mark explicit snapshots as experimental. I'm confident in this
  feature now (having finished the implementation in `classic-level`)
  and I'm happy with the API.

Ref: Level/community#118
Follow-Up-To: #93
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
semver-minor New features that are backward compatible
Projects
Level
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@vweevers