Remove allocation in server_auth filter by hcaseyal · Pull Request #15839 · grpc/grpc

hcaseyal · 2018-06-21T22:29:49Z

This PR adds a new API call for server_auth filter to allocate the server_security_context on the call's arena instead of doing a separate allocation.

grpc-testing · 2018-06-21T22:33:31Z

****************************************************************

libgrpc.so

     VM SIZE                                    FILE SIZE
 ++++++++++++++ GROWING                      ++++++++++++++
  +0.0%    +112 [None]                          +376  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc     +16  +0.1%
      +2.7%     +11 [Unmapped]                       +11  +2.7%
      [NEW]      +5 grpc_call_get_arena               +5  [NEW]

  +0.0%    +128 TOTAL                           +392  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-21T22:41:05Z

[trickle] No significant performance differences

grpc-testing · 2018-06-21T23:00:27Z

****************************************************************

libgrpc.so

     VM SIZE                                    FILE SIZE
 ++++++++++++++ GROWING                      ++++++++++++++
  +0.0%    +112 [None]                          +384  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc     +16  +0.1%
      +2.7%     +11 [Unmapped]                       +11  +2.7%
      [NEW]      +5 grpc_call_get_arena               +5  [NEW]

  +0.0%    +128 TOTAL                           +400  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-21T23:05:38Z

[trickle] No significant performance differences

grpc-testing · 2018-06-21T23:26:36Z

[microbenchmarks] No significant performance differences

ncteisen

As I reviewed I had an idea to restructure this.

Instead of changing the old API, leave that as is, but add a new one called grpc_server_security_context_create_from_arena

That also removes the need for the grpc_call_get_arena function

ncteisen · 2018-06-21T23:25:02Z

src/core/lib/surface/call.h

Please add comment that this is testing only and should not be used. In fact, if there is a way to accomplish this without adding this test function, we should

I'm not sure that just adding new function removes the need for the grpc_call_get_arena function. Since it's an internal test, let me chat with you offline about it.

yashykt · 2018-06-21T23:29:12Z

I would move the new function to a separate PR

grpc-testing · 2018-06-21T23:55:05Z

[microbenchmarks] No significant performance differences

yang-g · 2018-06-22T00:01:06Z

src/core/lib/security/context/security_context.h

what about the client side create, does it have the chance of arena allocation as well?

I will do client side in a separate PR (or I can do it in this PR after we decide on the correct way to handle server side).

grpc-testing · 2018-06-22T16:53:40Z

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.1%    +392 [None]                                              +1.05Ki  +0.0%
  +2.0%     +64 src/core/lib/security/context/security_context.cc       +64  +2.0%
      [NEW]     +42 destroy_server_ctx_internals                            +42  [NEW]
       +18%     +30 [Unmapped]                                              +30   +18%
      [NEW]     +10 grpc_server_security_context_create_from_arena          +10  [NEW]
      [NEW]      +5 grpc_server_security_context_destroy_from_arena          +5  [NEW]

  +0.0%    +456 TOTAL                                               +1.12Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T17:01:02Z

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.1%    +392 [None]                                              +1.05Ki  +0.0%
  +2.0%     +64 src/core/lib/security/context/security_context.cc       +64  +2.0%
      [NEW]     +42 destroy_server_ctx_internals                            +42  [NEW]
       +18%     +30 [Unmapped]                                              +30   +18%
      [NEW]     +10 grpc_server_security_context_create_from_arena          +10  [NEW]
      [NEW]      +5 grpc_server_security_context_destroy_from_arena          +5  [NEW]

  +0.0%    +456 TOTAL                                               +1.12Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T17:01:44Z

[trickle] No significant performance differences

grpc-testing · 2018-06-22T17:07:27Z

[trickle] No significant performance differences

grpc-testing · 2018-06-22T17:08:46Z

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.1%    +392 [None]                                              +1.05Ki  +0.0%
  +2.0%     +64 src/core/lib/security/context/security_context.cc       +64  +2.0%
      [NEW]     +42 destroy_server_ctx_internals                            +42  [NEW]
       +18%     +30 [Unmapped]                                              +30   +18%
      [NEW]     +10 grpc_server_security_context_create_from_arena          +10  [NEW]
      [NEW]      +5 grpc_server_security_context_destroy_from_arena          +5  [NEW]

  +0.0%    +456 TOTAL                                               +1.12Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

ncteisen · 2018-06-22T17:16:17Z

src/core/lib/security/context/security_context.h

One more nit: rather than include the arena in the header, can you forward declare it here, then only include it in the .cc file

Note google C++ style guide prefers #include to forward declaration :)

Welp, I didn't believe it, and I looked it up...

https://google.github.io/styleguide/cppguide.html#Forward_Declarations

I stand corrected. Hope, I have literally given you the opposite of good advice in this PR

grpc-testing · 2018-06-22T17:16:23Z

[trickle] No significant performance differences

markdroth · 2018-06-22T17:20:38Z

Are there cases where a security context is created other than for a specific call? If not, then I'm not sure I see the value in having an alternative ctor/dtor -- the original approach of just modifying the existing ctor/dtor to use the arena seems fine to me.

grpc-testing · 2018-06-22T17:30:47Z

****************************************************************

libgrpc.so

     VM SIZE                                                           FILE SIZE
 ++++++++++++++ GROWING                                             ++++++++++++++
  +0.1%    +392 [None]                                              +1.07Ki  +0.0%
  +2.0%     +64 src/core/lib/security/context/security_context.cc       +64  +2.0%
      [NEW]     +42 destroy_server_ctx_internals                            +42  [NEW]
       +18%     +30 [Unmapped]                                              +30   +18%
      [NEW]     +10 grpc_server_security_context_create_from_arena          +10  [NEW]
      [NEW]      +5 grpc_server_security_context_destroy_from_arena          +5  [NEW]

  +0.0%    +456 TOTAL                                               +1.13Ki  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T17:38:00Z

[trickle] No significant performance differences

hcaseyal · 2018-06-22T17:38:14Z

@markdroth It seems there are no other places where a security context is created besides for a specific call. However, there is an internal test that would need to access the call's arena in order to use the modified existing ctor. That's when I would need the get_call_arena function

grpc-testing · 2018-06-22T17:45:35Z

[microbenchmarks] No significant performance differences

markdroth · 2018-06-22T17:50:22Z

I think adding the grpc_call_get_arena() method is probably less messy than having multiple ctor/dtor for security context.

Alternatively, consider changing the internal test to create its own arena instead of trying to use the one from the call.

grpc-testing · 2018-06-22T17:53:32Z

[microbenchmarks] No significant performance differences

ncteisen · 2018-06-22T17:54:33Z

Adding grpc_call_get_arena to our surface API worries me a little... Hyrums law would say that if we do it, consumers of core will start allocating their stuff on the arena (which may not be a bad thing, but if it is a feature we should discuss)

markdroth · 2018-06-22T17:57:01Z

I'm not proposing that we add it to the external API. It should be declared in src/core/lib/surface/call.h, not in include/grpc/grpc.h. We already have a bunch of internal stuff in there, so I don't see why this is a problem.

grpc-testing · 2018-06-22T17:59:57Z

[microbenchmarks] No significant performance differences

ncteisen · 2018-06-22T18:04:07Z

Good point... sorry for the runaround Hope...

grpc-testing · 2018-06-22T18:22:02Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-22T18:33:29Z

****************************************************************

libgrpc.so

     VM SIZE                                    FILE SIZE
 ++++++++++++++ GROWING                      ++++++++++++++
  +0.0%    +112 [None]                          +376  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc     +16  +0.1%
      +2.7%     +11 [Unmapped]                       +11  +2.7%
      [NEW]      +5 grpc_call_get_arena               +5  [NEW]

  +0.0%    +128 TOTAL                           +392  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T18:56:49Z

[trickle] No significant performance differences

grpc-testing · 2018-06-22T19:26:27Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-22T19:41:36Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-22T20:30:26Z

****************************************************************

libgrpc.so

     VM SIZE                                                         FILE SIZE
 ++++++++++++++ GROWING                                           ++++++++++++++
  +0.0%     +96 [None]                                               +368  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc                          +16  +0.1%
      +2.7%     +11 [Unmapped]                                            +11  +2.7%
      [NEW]      +5 grpc_call_get_arena                                    +5  [NEW]

 -------------- SHRINKING                                         --------------
  -0.5%     -16 src/core/lib/security/context/security_context.cc     -16  -0.5%
      [DEL]     -10 grpc_server_security_context_create                   -10  [DEL]
      -5.1%      -9 [Unmapped]                                             -9  -5.1%
      -2.9%      -8 grpc_client_security_context_destroy                   -8  -2.9%

  +0.0%     +96 TOTAL                                                +368  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T20:37:57Z

[trickle] No significant performance differences

grpc-testing · 2018-06-22T20:46:12Z

****************************************************************

libgrpc.so

     VM SIZE                                                         FILE SIZE
 ++++++++++++++ GROWING                                           ++++++++++++++
  +0.0%     +96 [None]                                               +368  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc                          +16  +0.1%
      +2.7%     +11 [Unmapped]                                            +11  +2.7%
      [NEW]      +5 grpc_call_get_arena                                    +5  [NEW]

 -------------- SHRINKING                                         --------------
  -0.5%     -16 src/core/lib/security/context/security_context.cc     -16  -0.5%
      [DEL]     -10 grpc_server_security_context_create                   -10  [DEL]
      -5.1%      -9 [Unmapped]                                             -9  -5.1%
      -2.9%      -8 grpc_client_security_context_destroy                   -8  -2.9%

  +0.0%     +96 TOTAL                                                +368  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T20:53:34Z

[trickle] No significant performance differences

grpc-testing · 2018-06-22T21:21:55Z

[microbenchmarks] No significant performance differences

grpc-testing · 2018-06-22T21:37:58Z

[microbenchmarks] No significant performance differences

…tead

grpc-testing · 2018-06-22T22:53:51Z

****************************************************************

libgrpc.so

     VM SIZE                                    FILE SIZE
 ++++++++++++++ GROWING                      ++++++++++++++
  +0.0%    +112 [None]                          +376  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc     +16  +0.1%
      +2.7%     +11 [Unmapped]                       +11  +2.7%
      [NEW]      +5 grpc_call_get_arena               +5  [NEW]

  +0.0%    +128 TOTAL                           +392  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-22T23:02:13Z

[trickle] No significant performance differences

hcaseyal · 2018-06-22T23:16:42Z

Test flakes:
Bazel Opt build: #15827
Basic Tests MacOS: #15860
Bazel dbg build: #15861

grpc-testing · 2018-06-22T23:45:36Z

[microbenchmarks] Performance differences noted:
Benchmark                                                                                 cpu_time    real_time
----------------------------------------------------------------------------------------  ----------  -----------
BM_PumpStreamClientToServer<InProcessCHTTP2>/262144                                       -4%         -4%
BM_StreamingPingPong<MinInProcess, NoOpMutator, NoOpMutator>/262144/2                     +4%         +4%
BM_StreamingPingPongMsgs<InProcess, NoOpMutator, NoOpMutator>/32768                       +4%         +4%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/262144/1/0     +8%         +8%
BM_StreamingPingPongWithCoalescingApi<InProcess, NoOpMutator, NoOpMutator>/262144/2/1     +7%         +7%
BM_StreamingPingPongWithCoalescingApi<MinInProcess, NoOpMutator, NoOpMutator>/262144/1/0  +4%         +4%

grpc-testing · 2018-06-25T16:07:51Z

****************************************************************

libgrpc.so

     VM SIZE                                    FILE SIZE
 ++++++++++++++ GROWING                      ++++++++++++++
  +0.0%    +112 [None]                          +368  +0.0%
  +0.1%     +16 src/core/lib/surface/call.cc     +16  +0.1%
      +2.7%     +11 [Unmapped]                       +11  +2.7%
      [NEW]      +5 grpc_call_get_arena               +5  [NEW]

  +0.0%    +128 TOTAL                           +384  +0.0%


****************************************************************

libgrpc++.so

     VM SIZE        FILE SIZE
 ++++++++++++++  ++++++++++++++

  [ = ]       0        0  [ = ]

grpc-testing · 2018-06-25T17:00:55Z

[microbenchmarks] No significant performance differences

AspirinSJL · 2018-06-29T06:01:02Z

LGTM on the allocation from the arena.

hcaseyal requested a review from ncteisen June 21, 2018 23:10

ncteisen reviewed Jun 21, 2018

View reviewed changes

yang-g reviewed Jun 22, 2018

View reviewed changes

ncteisen reviewed Jun 22, 2018

View reviewed changes

ncteisen approved these changes Jun 22, 2018

View reviewed changes

hcaseyal force-pushed the server_auth_allocation branch 2 times, most recently from 101319f to f5805b7 Compare June 22, 2018 20:27

hcaseyal force-pushed the server_auth_allocation branch from f5805b7 to 0796789 Compare June 22, 2018 20:42

Remove allocation in server_auth filter and allocate on the arena ins…

8f3222c

…tead

hcaseyal force-pushed the server_auth_allocation branch from 0796789 to 8f3222c Compare June 22, 2018 22:50

hcaseyal mentioned this pull request Jun 25, 2018

Test flake: bins/dbg/h2_full+workarounds_nosec_test max_connection_age GRPC_POLL_STRATEGY=poll #15860

Closed

hcaseyal mentioned this pull request Jun 25, 2018

Test flake: //test/core/end2end:h2_full+workarounds_test@shutdown_finishes_calls@poller=epollex #15861

Closed

hcaseyal merged commit 618a3f5 into grpc:master Jun 25, 2018

hcaseyal deleted the server_auth_allocation branch June 25, 2018 18:41

hcaseyal assigned AspirinSJL Jun 28, 2018

AspirinSJL removed their assignment Jun 29, 2018

hcaseyal added the release notes: no Indicates if PR should not be in release notes label Jul 19, 2018

lock bot locked as resolved and limited conversation to collaborators Oct 17, 2018

Conversation

hcaseyal commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grpc-testing commented Jun 21, 2018

Uh oh!

grpc-testing commented Jun 21, 2018

Uh oh!

grpc-testing commented Jun 21, 2018

Uh oh!

grpc-testing commented Jun 21, 2018

Uh oh!

grpc-testing commented Jun 21, 2018

Uh oh!

ncteisen left a comment

Choose a reason for hiding this comment

Uh oh!

ncteisen Jun 21, 2018

Choose a reason for hiding this comment

Uh oh!

hcaseyal Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

yashykt commented Jun 21, 2018

Uh oh!

grpc-testing commented Jun 21, 2018

Uh oh!

yang-g Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

hcaseyal Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

ncteisen Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

yang-g Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

ncteisen Jun 22, 2018

Choose a reason for hiding this comment

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

markdroth commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

hcaseyal commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

markdroth commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

ncteisen commented Jun 22, 2018

Uh oh!

markdroth commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

ncteisen commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

grpc-testing commented Jun 22, 2018

Uh oh!

hcaseyal commented Jun 21, 2018 •

edited

Loading

hcaseyal commented Jun 22, 2018 •

edited

Loading