Nom parser #129

gsquire · Jul 8, 2017

This is an extremely rough start to using nom. I am sharing it to get thoughts on the layout as well as how to debug the unit tests. Perhaps I have written the parsers in a completely wrong way.

If this seems like it will not fit into this library, then feel free to close it. Thanks for looking.

Related to #81

gsquire · Jul 8, 2017

gsquire reviewed Jul 8, 2017

View changes

gsquire · Jul 8, 2017

src/parser.rs

-            '*' => self.parse_bulk(),
-            '-' => self.parse_error(),
-            _ => fail!((ErrorKind::ResponseError, "Invalid response when parsing value")),
+        let mut buf = [0; 4];


I need to figure out the best way to read from the socket. This is just for testing locally now.

@badboy Sorry to ping you directly, but I am curious if you find this PR valuable? If so I will keep iterating on it.

gsquire · Jul 13, 2017

@badboy Sorry to ping you directly, but I am curious if you find this PR valuable? If so I will keep iterating on it.

First, I appreciate your work here, regardless if we will merge it in the end.
I hadn't had time to take a look, but I will free up some time tomorrow and give you feedback then. Is that ok?

badboy · Jul 13, 2017

First, I appreciate your work here, regardless if we will merge it in the end.
I hadn't had time to take a look, but I will free up some time tomorrow and give you feedback then. Is that ok?

In any case I will keep working on it. I wanted to see if it would be a hard no or not :)

No rush on the feedback by the way! Whenever is convenient.

gsquire · Jul 13, 2017

In any case I will keep working on it. I wanted to see if it would be a hard no or not :)

No rush on the feedback by the way! Whenever is convenient.

@gsquire I am personally curious about performance difference between the current parser and your nom based one. Any chance you can provide some numbers?

jwilm · Jul 13, 2017

@gsquire I am personally curious about performance difference between the current parser and your nom based one. Any chance you can provide some numbers?

@jwilm I don't have any numbers I am confident in now and am more focused on correctness. If it proves to be the same or worse then I will abandon this.

gsquire · Jul 13, 2017

@jwilm I don't have any numbers I am confident in now and am more focused on correctness. If it proves to be the same or worse then I will abandon this.

I have to say...I am puzzled why reading from the socket causes the test to hang. The next step to try is a tcpdump of the traffic.

gsquire · Jul 14, 2017

I have to say...I am puzzled why reading from the socket causes the test to hang. The next step to try is a tcpdump of the traffic.

badboy · Jul 15, 2017

badboy reviewed Jul 15, 2017

View changes

badboy · Jul 15, 2017

src/parser.rs

-            '-' => self.parse_error(),
-            _ => fail!((ErrorKind::ResponseError, "Invalid response when parsing value")),
+        let mut buf = vec![];
+        match self.buf_reader.read_to_end(&mut buf) {


reading a stream to end will wait for an EOF, meaning for the connection to close.
That's definitely not what you want here.
You would want to read as much as possible, then try to parse it and if necessary read more of it.

I am working on this now, and am trying to figure out the most efficient way to do so.

badboy · Jul 15, 2017

badboy reviewed Jul 15, 2017

View changes

badboy · Jul 15, 2017

src/parser.rs

-            Some(detail) => fail!((kind, desc, detail.to_string())),
-            None => fail!((kind, desc)),
+fn parse_error(line: &str) -> RedisResult<Value> {
+    let desc = "An error was signaled by the server";


signalled

Hm, okay, both spellings are ok. Don't mind then.

It's definitely off to a good start.

@jwilm I don't have any numbers I am confident in now and am more focused on correctness. If it proves to be the same or worse then I will abandon this.

The current parser is pretty simple and not optimized at all. I'm sure there is some low-hanging fruit to speed it up.
However, we currently don't have any good benchmark for the parser. The 3 bundled benchmarks are are a starting point, but don't exercise most parts of the parser.

badboy · Jul 15, 2017

It's definitely off to a good start.

@jwilm I don't have any numbers I am confident in now and am more focused on correctness. If it proves to be the same or worse then I will abandon this.

The current parser is pretty simple and not optimized at all. I'm sure there is some low-hanging fruit to speed it up.
However, we currently don't have any good benchmark for the parser. The 3 bundled benchmarks are are a starting point, but don't exercise most parts of the parser.

I need to read up on nom and why I am getting Alt errors when feeding input. The unit tests I had written and tested separately all seemed happy before migrating it to this crate. I will keep digging into it.

gsquire · Jul 18, 2017

I need to read up on nom and why I am getting Alt errors when feeding input. The unit tests I had written and tested separately all seemed happy before migrating it to this crate. I will keep digging into it.

Another status update...

It's just hanging on the transaction tests now. I know the tests are failing but I will attend to that once I am done figuring out why it's not reading all that it should.

gsquire · Jul 22, 2017

Another status update...

It's just hanging on the transaction tests now. I know the tests are failing but I will attend to that once I am done figuring out why it's not reading all that it should.

@badboy I haven't had much time to look at this as I just moved into a new place. I still plan on getting this working and will return to it in due time. If you have any free time, you can edit my PR too. :)

gsquire · Aug 14, 2017

@badboy I haven't had much time to look at this as I just moved into a new place. I still plan on getting this working and will return to it in due time. If you have any free time, you can edit my PR too. :)

The transaction tests are failing on this line: https://github.com/mitsuhiko/redis-rs/pull/129/files#diff-2c09afcdc3c420ab0678ba9b5e83959cR35

The read is blocking for some reason and I cannot figure out why. After this I will see what the parser errors are.

gsquire · Aug 22, 2017

The transaction tests are failing on this line: https://github.com/mitsuhiko/redis-rs/pull/129/files#diff-2c09afcdc3c420ab0678ba9b5e83959cR35

The read is blocking for some reason and I cannot figure out why. After this I will see what the parser errors are.

I have squashed and rebased my changes.

For now I moved back to the old way of reading byte by byte since the buffered reader would just block for transactions. I would love to hear ideas as to why this could have been or what could be done to fix it. Also, there is a lot of refactoring I will get to as I haven't focused on code quality much to my chagrin.

Let me know your thoughts and thanks for your patience with this PR.

gsquire · Aug 26, 2017

I have squashed and rebased my changes.

For now I moved back to the old way of reading byte by byte since the buffered reader would just block for transactions. I would love to hear ideas as to why this could have been or what could be done to fix it. Also, there is a lot of refactoring I will get to as I haven't focused on code quality much to my chagrin.

Let me know your thoughts and thanks for your patience with this PR.

migrate to a nom based parser to help with efficiency

1cee1d3

Marwes referenced this pull request Jan 22, 2018
Merged
Add an async interface using futures-rs #141

gsquire closed this Feb 3, 2018

gsquire deleted the gsquire:nom-parser branch Feb 3, 2018

mitsuhiko/redis-rs

Nom parser #129

gsquire commented Jul 8, 2017 •

edited

Edited 1 time

gsquire edited Jul 8, 2017 (most recent)

gsquire reviewed Jul 8, 2017

View changes

This comment has been minimized.

This comment has been minimized.

gsquire commented Jul 13, 2017

This comment has been minimized.

badboy commented Jul 13, 2017

This comment has been minimized.

gsquire commented Jul 13, 2017

This comment has been minimized.

jwilm commented Jul 13, 2017

This comment has been minimized.

gsquire commented Jul 13, 2017

This comment has been minimized.

gsquire commented Jul 14, 2017

badboy reviewed Jul 15, 2017

View changes

This comment has been minimized.

This comment has been minimized.

badboy reviewed Jul 15, 2017

View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

badboy commented Jul 15, 2017

This comment has been minimized.

gsquire commented Jul 18, 2017

This comment has been minimized.

gsquire commented Jul 22, 2017

This comment has been minimized.

gsquire commented Aug 14, 2017

This comment has been minimized.

gsquire commented Aug 22, 2017

This comment has been minimized.

gsquire commented Aug 26, 2017 •

edited

Edited 1 time

gsquire edited Aug 27, 2017 (most recent)

Marwes referenced this pull request Jan 22, 2018

Add an async interface using futures-rs #141

gsquire closed this Feb 3, 2018

gsquire deleted the gsquire:nom-parser branch Feb 3, 2018

mitsuhiko/redis-rs

Join GitHub today

Nom parser #129

Conversation

gsquire commented Jul 8, 2017 • edited Edited 1 time gsquire edited Jul 8, 2017 (most recent)

gsquire reviewed Jul 8, 2017 View changes

This comment has been minimized.

This comment has been minimized.

gsquire commented Jul 13, 2017

This comment has been minimized.

badboy commented Jul 13, 2017

This comment has been minimized.

gsquire commented Jul 13, 2017

This comment has been minimized.

jwilm commented Jul 13, 2017

This comment has been minimized.

gsquire commented Jul 13, 2017

This comment has been minimized.

gsquire commented Jul 14, 2017

badboy reviewed Jul 15, 2017 View changes

This comment has been minimized.

This comment has been minimized.

badboy reviewed Jul 15, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

badboy commented Jul 15, 2017

This comment has been minimized.

gsquire commented Jul 18, 2017

This comment has been minimized.

gsquire commented Jul 22, 2017

This comment has been minimized.

gsquire commented Aug 14, 2017

This comment has been minimized.

gsquire commented Aug 22, 2017

This comment has been minimized.

gsquire commented Aug 26, 2017 • edited Edited 1 time gsquire edited Aug 27, 2017 (most recent)

Marwes referenced this pull request Jan 22, 2018

gsquire closed this Feb 3, 2018

gsquire deleted the gsquire:nom-parser branch Feb 3, 2018

gsquire commented Jul 8, 2017 •

edited

Edited 1 time

gsquire edited Jul 8, 2017 (most recent)

gsquire reviewed Jul 8, 2017

View changes

badboy reviewed Jul 15, 2017

View changes

badboy reviewed Jul 15, 2017

View changes

gsquire commented Aug 26, 2017 •

edited

Edited 1 time

gsquire edited Aug 27, 2017 (most recent)