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

Breaking: bump abstract-level to 2.0.0 #52

Merged
merged 12 commits into from
Oct 20, 2024
Merged

Breaking: bump abstract-level to 2.0.0 #52

merged 12 commits into from
Oct 20, 2024

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Nov 18, 2022

TODO:

  • Pass abstract test suite
  • Update additional tests
  • Update types
  • Cleanup the closing of iterators (see TODO comments in binding)
  • Update docs
  • Write upgrade guide
  • Create snapshot for get() synchronously (Tracking issue: implicit and explicit snapshots community#118)
  • Support signal option on iterators
  • Benchmark (optional):
    • get() concurrently, to see if synchronous snapshot blocks event loop
    • iterator(), to compare callbacks vs promises
    • put(), to compare callbacks vs promises

To be rebased (not fully squashed) before merge.

@vweevers vweevers added the semver-major Changes that break backward compatibility label Nov 18, 2022
@vweevers vweevers changed the title Bump abstract-level to 2.0.0 BreakingBump abstract-level to 2.0.0 Nov 18, 2022
@vweevers vweevers changed the title BreakingBump abstract-level to 2.0.0 Breaking: bump abstract-level to 2.0.0 Nov 18, 2022
@vweevers vweevers marked this pull request as ready for review February 4, 2024 19:03
@ishfx
Copy link

ishfx commented Mar 4, 2024

Hey, when do you plan to merge/release v2? Looking forward to it, thanks!

@jacoscaz
Copy link

@vweevers would you like some help with benchmarking?

@vweevers
Copy link
Member Author

Help is always welcome, thank you! Though FYI, that's not why I haven't released this yet. Just haven't had the time, and I don't consider benchmarks to be a blocker (because e.g. promises are unlikely to make a big difference here) so I planned to just skip them when I got to it.

The only thing that slightly worries me is that db.get() now creates a snapshot synchronously. I don't know if that's a fast operation for LevelDB.

@jacoscaz
Copy link

@vweevers no pressure meant or implied. I depend on level packages through quadstore and I'd be happy to help an upstream dependency if I can find the time.

Also, my experience with promise-based iteration is exactly the opposite; so far I've always found it to make a rather big difference, particularly with asynchronous sources capable of reading and returning multiple items in batches and particularly when structuring chains of iterators. That said, I also recognize that I'm not as experienced as you are. I'll have a look next week!

@vweevers
Copy link
Member Author

I'm not that experienced with promises themselves, I meant more that disk and the C<>JS barrier are the bigger bottlenecks here. But it's always good to challenge such assumptions. You're probably right that there's an overhead to e.g. async iterators.

For people that were already using promises, I do hope that abstract-level v2 and classic-level v2 are faster because they no longer have to translate callbacks into promises, allowing V8 to optimize it.

Has a tiny performance cost, which I negated by optimizing the
passing of options from JS to C++. The end result is faster than
before. However, I didn't check if it blocks the event loop for
a significant amount of time. Benchmarking concurrent gets might
answer that, later.

Ref: Level/community#118
Category: fix
This is a new optional feature in `abstract-level` 2.0.0.

Category: addition
Too much hassle. If you use FreeBSD and would like to see this
restored then please reach out. It will require a commitment to
help with maintenance and any issues that may arise.

Category: removal
Instead of copying abstract-level docs and then tweaking it for
classic-level (on every release, which was a bit of a pain) the
README now only describes the differences.

Category: change
@vweevers
Copy link
Member Author

To be rebased (not fully squashed) before merge.

Done. I squashed some commits, tweaked descriptions. This push contains no code changes besides a typo and changing bool aborted_ to std::atomic<bool> aborted_.

Considering the amount of time passed since I opened this PR, I want to revisit CI, e.g. to drop Node.js 16 and bump the electron version we test against. I'll do that in a separate PR.

@vweevers
Copy link
Member Author

Canceling CI because macos jobs are stuck in the queue for 10+ minutes.

@vweevers vweevers merged commit e5ef03e into main Oct 20, 2024
18 of 24 checks passed
@vweevers vweevers deleted the v2 branch October 20, 2024 15:27
@vweevers
Copy link
Member Author

2.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants