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.

Sign up for GitHub

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

Jump to bottom

Completion Queue creation API changes (C-Core and C++ only) #9972

Merged
merged 84 commits into from
Apr 14, 2017
Merged

Completion Queue creation API changes (C-Core and C++ only) #9972

merged 84 commits into from
Apr 14, 2017

Conversation

sreecha
Copy link
Contributor

@sreecha sreecha commented Mar 3, 2017 •
edited
Loading

This is the initial pull request for grpc_completion_queue_create changes as listed in the gRFC grpc/proposal#6 . These were the minimum set of changes I could make to get this to compile and for tests (C/C++) to pass :) [Note: This has all the API changes needed. Its just that I resisted the temptation to implement a bit more functionality]

Changes in this PR

  • Breaking change to the grpc_completion_queue_create API in C-Core

  • Added asserts in grpc_completion_queue_next and grpc_completion_queue_pluck to make sure they are called on the completion queues with correct grpc_cq_completion_type

  • Did not make any other functionality changes in core (like for example not creating a pollset if grpc_cq_polling_type is NON_POLLING). This will happen in a separate PR

  • No changes to the C++ user-facing API. These will happen in later PRs (when I work on hybrid servers). The main change in C++ was to make sure that completion queues created for Calls were of GRPC_CQ_PLUCK type and all other completion queues were of GRPC_CQ_NEXT type (the polling type for all these queues is DEFAULT_POLLING). Will make more changes later.

Changes in other wrapped languages

UPDATE
Update on 3/22/2017: Pulled in the changes from #10245 as well.

This change is Reviewable

@ctiller
Copy link
Member

ctiller commented Mar 3, 2017

I like the idea of prefixing the enum names.

This was referenced Mar 6, 2017
Merged
Merged
@sreecha
Copy link
Contributor Author

sreecha commented Mar 6, 2017 •
edited
Loading

@murgatroid99 and @muxi: Can you please take a look at this PR ? sreecha#5
(Muxi for objective C changes and Michael for Ruby and Node)
If everything looks good, I'll merge that with this current PR

@kpayson64 , can you please take a look at Python changes here: sreecha#4

@sreecha
Php: Completion queue create API changes (and also remove the redundant
5fcb8cc 
completion_queue_next() loop in grpc_php_shutdown_completion_queue
function as it is no longer needed. This was most likely an artifact of
a previous version of grpc-core where completion queues did not have a
"one tag in, one tag out" rule
@sreecha
Copy link
Contributor Author

sreecha commented Mar 7, 2017

@stanley-cheung : Can you please take a look at the PR here: sreecha#6 ?

@grpc-kokoro
Copy link 

Performance differences noted:
Benchmark                      allocs_per_iteration    atm_add_per_iteration    atm_cas_per_iteration      cpu_time  locks_per_iteration      real_time  writes_per_iteration
-----------------------------  ----------------------  -----------------------  -----------------------  ----------  ---------------------  -----------  ----------------------
BM_ErrorCreateAndSetIntAndStr                                                                               +285.50                             +285.50
BM_ErrorCreateAndSetIntLoop                                                                                  +10.50                              +10.50
BM_ErrorCreateAndSetStatus                                                                                  +246.00                             +246.00
BM_ErrorCreateAndSetStrLoop                                                                                  +25.00                              +25.00
BM_ErrorCreateFromCopied                                                                                    +263.00                             +263.00
BM_ErrorCreateFromStatic                                                                                    +219.00                             +219.00

@grpc-kokoro
Copy link 

No significant performance differences

@grpc-kokoro
Copy link 

Performance differences noted:
Benchmark                                                  allocs_per_iteration    atm_add_per_iteration    atm_cas_per_iteration      cpu_time  locks_per_iteration      real_time  writes_per_iteration
---------------------------------------------------------  ----------------------  -----------------------  -----------------------  ----------  ---------------------  -----------  ----------------------
BM_ErrorGetStatus<ErrorCancelled>                                                                                                        -21.50                              -21.50
BM_ErrorGetStatus<ErrorWithHttpError>                                                                                                    -36.00                              -36.00
BM_ErrorGetStatusCode<ErrorWithNestedGrpcStatus>                                                                                         -15.50                              -15.50
BM_HpackParserParseHeader<AddIndexedSingleStaticElem>                                                                                    -44.00                              -44.00
BM_IsolatedFilter<LoadReportingFilter, SendEmptyMetadata>                                                                                +12.50                              +12.50

@grpc-kokoro
Copy link 

No significant performance differences

@grpc-kokoro
Copy link 

No significant performance differences

@ctiller ctiller mentioned this pull request Apr 11, 2017
Merged
@grpc-kokoro
Copy link 

Performance differences noted:
Benchmark                                                  cpu_time    real_time
-------------------------------------------------------  ----------  -----------
BM_CallCreateDestroy<LameChannel>                            +79.50       +79.50
BM_CreateDestroyCore                                         +23.50       +23.50
BM_CreateDestroyCpp2                                         +20.00       +20.00
BM_ErrorCreateFromCopied                                     +49.50       +49.50
BM_ErrorStringOnNewError<ErrorWithHttpError>                +393.00      +393.00
BM_IsolatedFilter<ClientDeadlineFilter, NoOp>                 +5.50        +5.50
BM_IsolatedFilter<MessageSizeFilter, SendEmptyMetadata>       +9.00        +9.00
BM_IsolatedFilter<ServerDeadlineFilter, NoOp>                 +6.00        +6.00
BM_LameChannelCallCreateCpp                                 +570.00      +570.00
BM_Pass1Core                                                 +20.50       +20.50
BM_Pass1Cpp                                                  +27.00       +27.00
BM_Pluck1Core                                                +16.00       +16.00
BM_Zalloc/4k                                                 +17.50       +17.50
BM_Zalloc/6k                                                 +22.00       +22.00
BM_Zalloc/7k                                                 +30.00       +30.00

@grpc-kokoro
Copy link 

Performance differences noted:
Benchmark                                                                     cpu_time    real_time
--------------------------------------------------------------------------  ----------  -----------
BM_HpackEncoderEncodeHeader<SingleNonInternedBinaryElem<10, false>>/0/16k       +70.00       +70.00
BM_HpackEncoderEncodeHeader<SingleNonInternedBinaryElem<100, false>>/0/16k     +100.50      +100.50
BM_HpackEncoderEncodeHeader<SingleNonInternedBinaryElem<31, false>>/0/16k       +72.00       +72.00
BM_HpackParserParseHeader<NonIndexedBinaryElem<1, false>>                      +107.50      +107.50
BM_HpackParserParseHeader<NonIndexedBinaryElem<1, true>>                       +109.50      +109.50
BM_HpackParserParseHeader<NonIndexedBinaryElem<10, false>>                     +104.00      +104.00
BM_HpackParserParseHeader<NonIndexedBinaryElem<10, true>>                      +119.00      +119.00
BM_HpackParserParseHeader<NonIndexedBinaryElem<100, true>>                     +105.50      +105.50
BM_HpackParserParseHeader<NonIndexedBinaryElem<3, false>>                      +105.50      +105.50
BM_HpackParserParseHeader<NonIndexedBinaryElem<3, true>>                        +96.50       +96.50
BM_HpackParserParseHeader<NonIndexedBinaryElem<31, false>>                     +120.00      +120.00
BM_HpackParserParseHeader<NonIndexedBinaryElem<31, true>>                      +113.00      +113.00
BM_HpackParserParseHeader<NonIndexedElem>                                      +101.50      +101.50

@grpc-kokoro
Copy link 

Performance differences noted:
Benchmark                                                                  cpu_time    real_time
-----------------------------------------------------------------------  ----------  -----------
BM_ErrorGetStatus<ErrorNone>                                                  -5.50        -5.50
BM_ErrorGetStatus<SimpleError>                                                -6.00        -6.00
BM_ErrorHttpError<ErrorNone>                                                  -5.50        -5.50
BM_HpackEncoderEncodeHeader<RepresentativeServerTrailingMetadata>/1/16k       +8.00        +8.00
BM_HpackEncoderEncodeHeader<SingleNonInternedBinaryElem<3, true>>/0/16k      -16.50       -16.50
BM_SingleThreadPollOneFd                                                    -128.00      -127.50
BM_SingleThreadPollOneFd_SpeedOfLight                                        -56.00       -56.00
BM_TransportStreamRecv/16M                                                +34014.50    +34011.50
BM_TransportStreamRecv/256k                                                 +593.50      +594.00
BM_TransportStreamRecv/2M                                                  +4509.50     +4509.50
BM_TransportStreamSend/16M                                                +16310.50    +16313.00
BM_Zalloc/256                                                                +10.00       +10.00
BM_Zalloc/512                                                                +14.50       +14.50
BM_Zalloc/7k                                                                 +54.50       +54.50

@grpc-kokoro
Copy link 

No significant performance differences

@grpc-kokoro
Copy link 

Performance differences noted:
Benchmark                                                                  cpu_time    real_time
-----------------------------------------------------------------------  ----------  -----------
BM_HpackEncoderEncodeHeader<EmptyBatch>/0/16k                                 -7.00        -7.00
BM_HpackEncoderEncodeHeader<RepresentativeServerInitialMetadata>/0/16k       +13.00       +13.00
BM_HpackEncoderEncodeHeader<RepresentativeServerTrailingMetadata>/1/16k       +8.50        +8.50
BM_IsolatedFilter<MessageSizeFilter, NoOp>                                    -5.50        -5.50
BM_LameChannelCallCreateCpp                                                -1392.50     -1393.00
BM_SliceIntern                                                               +20.50       +20.50

@sreecha sreecha merged commit b81fb79 into grpc:master Apr 14, 2017
@sreecha
Copy link
Contributor Author

sreecha commented Apr 14, 2017

Interop failures and bazel failures are known

@sreecha sreecha mentioned this pull request Apr 14, 2017
Merged
@sreecha sreecha deleted the cq_create_api_changes branch April 21, 2017 22:09
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ctiller ctiller ctiller approved these changes

Assignees

@ctiller ctiller

Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sreecha @ctiller @grpc-kokoro @googlebot @jtattermusch