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

Added a stream for non-cloneable iterators #37

Merged
merged 3 commits into from
Oct 7, 2015
Merged

Conversation

Marwes
Copy link
Owner

@Marwes Marwes commented Jul 16, 2015

This PR sketches out a possible implementation for a stream which do not directly support cloning. It does so by storing the tokens read from the iterator into a circular buffer which only allows for partial backtracking. Using this additional indirection iterators which cannot be cloned can be used as streams.

The performance drop using this stream type was measured to be about 20% for the JSON parser in the benches folder. Reading directly from a file or socket is not tested but it should work though the chars iterator on Read (though it is currently unstable).

I will hold on to merging this for a while as I would really appreciate some feedback on this as I do not intend to use this myself (and it needs documentation).

cc #30

@Marwes Marwes changed the title Added a stream which allows for non-cloneable iterators to be used Added a stream for non-cloneable iterators Jul 16, 2015
@Marwes Marwes mentioned this pull request Jul 16, 2015
@hawkw
Copy link
Contributor

hawkw commented Jul 16, 2015

This definitely seems like a useful thing to have!

@Marwes
Copy link
Owner Author

Marwes commented Jul 16, 2015

@hawkw Yeah, it could be implemented outside the library but it is probably useful enough (and isn't trivial to implement) to warrant inclusion.

I realize now that io::Chars has Result<char, CharsError> as items which should proabably be handled but it cannot be done with the current API. It would probably be a good idea to handle the error case as well in some way.

@Marwes
Copy link
Owner Author

Marwes commented Jul 16, 2015

And one more thing as food for thought for anyone reading this. I used & references to share the buffer for efficiency's sake which is probably what you want in any case, would there be any use for a variant which shares it through Rc instead? (It strikes me as similar to the PR is submitted to zslayton/lifeguard#4 which has both).

A problem with using & references seems to be that the buffered stream must strictly outlive the parsers which consume it (which bit me while writing the tests but shouldn't prevent you from writing any actual code).

@Marwes Marwes added this to the 1.1 milestone Aug 2, 2015
@Marwes
Copy link
Owner Author

Marwes commented Aug 2, 2015

To make sure I can release a stable 1.0 within a week or two I will hold of with this PR until shortly after 1.0 when I will put this and probably range streams in a 1.1-beta version where it can be better tested.

#42

Marwes added a commit that referenced this pull request Oct 7, 2015
Added a stream for non-cloneable iterators
@Marwes Marwes merged commit 1eccea7 into master Oct 7, 2015
@Marwes Marwes deleted the buffered_stream branch October 17, 2015 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants