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

Remove redundant work serializer usage in c-ares windows code #28016

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

apolcyn
Copy link
Contributor

@apolcyn apolcyn commented Nov 11, 2021

This is extracted from the changes to c-ares windows code in https://github.com/grpc/grpc/pull/27858/files.

I'm not 100% sure why we were explicitly scheduling these methods on the work serializer when we were already running under it.

I think it's because these methods were originally added back in 03797e0, which was before the change to use WorkSerializers in e05417d#diff-22089b08a43cdad7a2fcf577721907568fe29a57024bf600d137c69996fed567. In the former PR, the pending_continue_register_for_on_{readable,writable}_locked_ fields were pointers to closures, and code would schedule their underlying closures on the combiner, even if we were already running under the combiner. The change to use work serializer also changed these fields from pointers to booleans, and also changed code to explicitly schedule their methods on the work serializer in the case these flags were set. But, explicitly scheduling on the work serializer remained redundant.

@apolcyn apolcyn added lang/core release notes: yes Indicates if PR needs to be in release notes labels Nov 11, 2021
@apolcyn
Copy link
Contributor Author

apolcyn commented Nov 12, 2021

portability tests windows: #28025

basic tests objc: #21858

@apolcyn apolcyn enabled auto-merge (squash) November 12, 2021 17:38
@apolcyn
Copy link
Contributor Author

apolcyn commented Nov 16, 2021

Easy CLA is holding this up. Trying to re-kick it.

@apolcyn
Copy link
Contributor Author

apolcyn commented Nov 17, 2021

I signed it

@apolcyn
Copy link
Contributor Author

apolcyn commented Nov 29, 2021

/easycla

@apolcyn apolcyn merged commit cce34f6 into grpc:master Nov 29, 2021
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core perf-change/none release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants