-
Notifications
You must be signed in to change notification settings - Fork 421
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 channel.advancePastByte, use it to improve revcomp #8103
Conversation
// Scan forward until we get to the > (end of sequence) | ||
input.advancePastByte(ascii(">")); | ||
nextDescOffset = input._offset(); | ||
} catch UnexpectedEOFError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the compiler currently support this pattern? I think it should, but right now it might need to be catch e: UnexpectedEOFError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems to work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet that it's not matching on the type but rather I'm naming an error variable UnexpectedEOFError
and it shadows the type. I.e. it's like I wrote catch e
. I'll update it.
Stab in the dark to head off potential testing noise: does the need for PR #8116 have any implications for this one? |
This version of the benchmark finds the terminators within the I/O buffer and then reads the appropriate amount of data directly.
ec38ba9
to
1279813
Compare
@benharsh - could you review this PR? we certainly talked about it. It passed full local testing in an earlier version - after review I plan to run that again. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Some potential follow-up issues to think about:
- This pattern feels different enough from other languages to warrant some kind of simple example somewhere.
- It would be nice if we could pass arrays (at least DefaultRectangular) to
channel.readBytes
. This is probably a pretty simple change.
PR #8045 added some messy rev-comp studies that improve performance by improving the I/O pattern. What the performance comes down to is two things:
I experimented with a version that used regexp format strings to replace the memchr call but that had unsatisfying performance.
This PR adds channel.advancePastByte in order to enable the expression of the fast I/O pattern in revcomp easily. Now the revcomp version does the following:
I'm seeing a 10% speedup for this version beyond revcomp-buf.chpl, and it is much simpler.
While there, I noticed that qio_channel_advance might not set up the buffer in some cases, so added code to do that.
Closes #8105.
Reviewed by @benharsh - thanks!