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 skip for ExternalIterator #1725

Closed
webmaster128 opened this issue Jun 15, 2023 · 5 comments · Fixed by #1838
Closed

Implement skip for ExternalIterator #1725

webmaster128 opened this issue Jun 15, 2023 · 5 comments · Fixed by #1838

Comments

@webmaster128
Copy link
Member

Right now skipping elements decodes the result region which we don't need as the value is dropped. We can just do

        let next_result = unsafe { db_next(self.iterator_id) };
        let kv_region_ptr = next_result as *mut Region;
        let _kv = unsafe { consume_region(kv_region_ptr) };

instead.

This improvement alone is not a game changer but in a follow-up step we can then add a db_skip(self.iterator_id) low import that does not write key and value to Wasm at all.

@webmaster128 webmaster128 modified the milestones: 1.3.0, 1.4.0 Jun 15, 2023
@ethanfrey
Copy link
Member

Huh, you want to override the trait method on the Rust side. Is this called somewhere? (We use limit a lot of places, but I don't think that calls skip. What composition would call this?)

Also seems they recommend overriding nth: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.skip

@webmaster128
Copy link
Member Author

webmaster128 commented Jun 21, 2023

You need this when doing iteration with offsets (pagination). Right now it just calls next for every skipped element, which is horribly inefficient. Once we have a custom implementation (skip or nth, depending on what makes sense here), we will see that this can be improved by having a db_skip import that does not load the unused key/value into Wasm.

@ethanfrey
Copy link
Member

ethanfrey commented Jun 21, 2023

Oh, who uses pagination with numeric offsets? That is always terribly inefficient.
I learned that back in web land when handling large collections backed by PostgreSQL.

Proper way is to provide some cursor like start_after which can be translated to the last key show to efficiently iterate after that. Whoever is doing this other approach should learn better design.

Not saying we can't do this, but it is covering up a problematic design... and they will still call the IAVL tree all those times, with 1000+ gas for every value they skip, even if this doesn't cause contract overhead.

@webmaster128
Copy link
Member Author

Yeah I know, I know. It was just the first example that popped up in my mind. I'll come up with a better use case ...

@webmaster128
Copy link
Member Author

This is implemented in #1838. However, Ethan is right and we should be careful when this is actually used. It indicates a bad storage layout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants