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

Add sync execution methods and performance optimization. #136

Closed
wants to merge 15 commits into from

Conversation

snowyu
Copy link

@snowyu snowyu commented Dec 1, 2014

  • Add the OpenSync, GetSync, PutSync, DeleteSync, BatchSync, Iterator.NextSync, Iterator.EndSync
  • Optimize the iterator performance and memory-leak fixed
  • Add the bench/bench.js to compare
  • Using the new snowyu/abstract-leveldown#feature/sync to test

[Edit: the link to the discussion in abstract-leveldon is https://github.com/Level/abstract-leveldown/pull/49 - @mcollina]

@ralphtheninja
Copy link
Member

A couple of things:

  • What is the reason for all these synchronous methods?
  • This is a quite a big PR. Is there any way you could split it up into smaller changes so they can be discussed more easily? Updating leveldb version should really be a separate PR imo.
  • I noticed some commits were erroneous (the ones related to Travis). It would be nice if you could prune them.

@mcollina
Copy link
Member

mcollina commented Dec 1, 2014

👍 on separate PRs.

I really like the openSync method, I understand why you might want to provide getSync and putSync, but I am missing the point for the iterator. It's as fast as the non-sync one, and it just adds code to maintain.

@snowyu
Copy link
Author

snowyu commented Dec 2, 2014

in fact , I just wanna the OpenSync, and GetSync are enough for me.

iterator sync just as compare test :).

@snowyu
Copy link
Author

snowyu commented Dec 2, 2014

But leave these choose to others, not me to choose for them.

@snowyu
Copy link
Author

snowyu commented Dec 6, 2014

@ralphtheninja: already rebased.

I've publish it as leveldown-sync for my requirement. also levelup-sync and memdown-sync made.

@ralphtheninja
Copy link
Member

This would have to be re-implemented since we're now based on n-api.

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.

3 participants