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

LevelDOWN's chained batch #137

Closed
wants to merge 2 commits into from
Closed

LevelDOWN's chained batch #137

wants to merge 2 commits into from

Conversation

juliangruber
Copy link
Member

I added an implementation of LevelDOWN's chained batch with support for encodings and events.

This has a basic test but still needs tests for encodings and events.

Also, I think the Batch class should be moved to lib/batch.js which then requires a refactor of dispatchError since Batch needs that too.

@rvagg
Copy link
Member

rvagg commented May 11, 2013

yeah, move it to a separate file.

The this.ops array is unfortunate and bound to slow performance down, not that I can see a way around that but it sort makes me wonder what the point of doing it the "proper" way in leveldown is/was. Why not just use db.batch(this.ops) at the end?

@rvagg
Copy link
Member

rvagg commented May 11, 2013

perhaps this.ops isn't that bad, it's just an array of existing objects after all, there's no copying going on or anything.

@juliangruber can you think up a quick way to benchmark this to see if we're winning anything here?

@juliangruber
Copy link
Member Author

I'll create a quick bench

@juliangruber
Copy link
Member Author

old batch: 386.69760247486465 ops/s
new batch: 529.6610169491526 ops/s

~36% faster

@rvagg
Copy link
Member

rvagg commented May 11, 2013

#win lets review this and get it in for 0.9 eh?

@maxogden @No9 this is going to mess up AbstractLevelDOWN for you by the way, the batch() tests in there are incomplete because we've been waiting for the new batch style to be exposed up here, but they're in LevelDOWN already. Your code will need to change to handle a chained batch as well as the current array-style of batch.

@rvagg rvagg mentioned this pull request May 14, 2013
@rvagg
Copy link
Member

rvagg commented May 23, 2013

merged

@rvagg rvagg closed this May 23, 2013
@juliangruber juliangruber deleted the chained-leveldown branch June 21, 2014 06:14
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.

2 participants