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

#1upx91t fix (cpp): Modify ASRStream implementation to better respond to results. #70

Merged
merged 1 commit into from Jan 19, 2022

Conversation

mplooster
Copy link
Contributor

It turns out that the bool returned by the underlying gRPC stream
Write() method, didn't quite mean what I thought it meant (C++ API
docs aren't very clear on this point). The Write() method will
return false when the stream is closed, but not necessarily when
the result is available. However, the result IS available as soon
as the stream's Finish() method returns. Experiments confirm that
this function can return a good 400-500ms before the Write() method
returns false. So, to improve the notification of when a result is
available, the ASRStream implementation was updated to listen for
a result (via the blocking Finish method) on a separate thread.
Using an atomic bool as a flag, we then let the calling thread know
when a result is available by returning false from the ASRStream's
send methods once the flag is set. This greatly improves the time
it takes for a client to get the ASR result.

Also simplified the creation of the ASRStream by passing in the
client Stub pointer, and better organized the class by storing
internal class data in an opaque pointer (also helps with issues
related to storing a std::thread object in a class).

It turns out that the bool returned by the underlying gRPC stream
Write() method, didn't quite mean what I thought it meant (C++ API
docs aren't very clear on this point). The Write() method will
return false when the stream is closed, but not necessarily when
the result is available. However, the result IS available as soon
as the stream's Finish() method returns. Experiments confirm that
this function can return a good 400-500ms before the Write() method
returns false. So, to improve the notification of when a result is
available, the ASRStream implementation was updated to listen for
a result (via the blocking Finish method) on a separate thread.
Using an atomic bool as a flag, we then let the calling thread know
when a result is available by returning false from the ASRStream's
send methods once the flag is set. This greatly improves the time
it takes for a client to get the ASR result.

Also simplified the creation of the ASRStream by passing in the
client Stub pointer, and better organized the class by storing
internal class data in an opaque pointer (also helps with issues
related to storing a std::thread object in a class).
@cobaltjenkins
Copy link

This PR is not linked to a task in ClickUp. Please add the task ID starting with "CU-" (e.g. CU-1umz2q2) as the first word in the title.

To create a new task in the list click here.

@render
Copy link

render bot commented Jan 18, 2022

Copy link

@happyalu happyalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! must have been a nightmare figuring this out.

@cobaltjenkins
Copy link

Please add the ClickUp ID to the PR title

@cobaltjenkins
Copy link

This PR is not linked to a task in ClickUp. Please add the task ID starting with "CU-" (e.g. CU-1umz2q2) as the first word in the title.

To create a new task in the list click here.

@renaissanceGeek renaissanceGeek changed the title fix (cpp): Modify ASRStream implementation to better respond to results. #1upx91t fix (cpp): Modify ASRStream implementation to better respond to results. Jan 18, 2022
@cobaltjenkins
Copy link

Copy link
Contributor

@renaissanceGeek renaissanceGeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Great job finding it and fixing it in a thread-safe manner!

@mplooster mplooster merged commit 2880808 into master Jan 19, 2022
@mplooster mplooster deleted the mplooster-cpp-asrstream branch January 19, 2022 00:49
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.

None yet

4 participants