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

Introduce MIXED SSL Provider #9617

Closed
wants to merge 1 commit into from

Conversation

vkostyukov
Copy link
Contributor

@vkostyukov vkostyukov commented Sep 27, 2019

This is an exploratory PR (WIP) and I'm curious to hear what people think about this new feature.

This was a hack week at Twitter and my (very boring) project was to see what we can gain by dropping them finalizers on SslEgines. One of the long-standing problems we had with services churning connections is their GC times are suffering from very long finalizer queues. One workaround we applied was to use parallel ref processing. This mitigated the problem but didn't solve the problem entirely.

You'd ask why we're not using RefCounted OpenSSL engines - this is exactly what they exist for. Well, we want but we can't due to how we instantiate SSL contexts in Finagle (we can properly release engines but not contexts).

I tried to implement a MIXIED SSL provider that suites our needs - it uses finalizers for contexts but relies on manual release for engines. I vendored a Netty internally and tried the branch against a couple of services.

The results are promising. Here is an example of a "remark" GC phase BEFORE and AFTER (we used to see a lot of slowdown during remark):

BEFORE:

                                   num  min/ms  p50/ms  p90/ms  p99/ms  max/ms  tot/sec
GC_Remark_Post                       1   29.70   29.70   29.70   29.70   29.70     0.03  100.00%
  GC_RefProc                         1   28.72   28.72   28.72   28.72   28.72     0.03   96.70%
    GC_RefProc_ProcessSoftRefs       1    0.94    0.94    0.94    0.94    0.94     0.00    3.16%
    GC_RefProc_ProcessWeakRefs       1    2.28    2.28    2.28    2.28    2.28     0.00    7.68%
    GC_RefProc_ProcessFinalRefs      1   24.13   24.13   24.13   24.13   24.13     0.02   81.25%
    GC_RefProc_ProcessPhantomRefs    1    0.46    0.46    0.46    0.46    0.46     0.00    1.55%
    GC_RefProc_ProcessCleaners       1    0.55    0.55    0.55    0.55    0.55     0.00    1.85%
    GC_RefProc_ProcessJNIWeakRefs    1    0.28    0.28    0.28    0.28    0.28     0.00    0.94%

AFTER:

                                   num  min/ms  p50/ms  p90/ms  p99/ms  max/ms  tot/sec
GC_Remark_Post                       1    7.29    7.29    7.29    7.29    7.29     0.01  100.00%
  GC_RefProc                         1    6.84    6.84    6.84    6.84    6.84     0.01   93.83%
    GC_RefProc_ProcessSoftRefs       1    0.64    0.64    0.64    0.64    0.64     0.00    8.78%
    GC_RefProc_ProcessWeakRefs       1    4.54    4.54    4.54    4.54    4.54     0.00   62.28%
    GC_RefProc_ProcessFinalRefs      1    0.51    0.51    0.51    0.51    0.51     0.00    7.00%
    GC_RefProc_ProcessPhantomRefs    1    0.46    0.46    0.46    0.46    0.46     0.00    6.31%
    GC_RefProc_ProcessCleaners       1    0.46    0.46    0.46    0.46    0.46     0.00    6.31%
    GC_RefProc_ProcessJNIWeakRefs    1    0.17    0.17    0.17    0.17    0.17     0.00    2.33%

You can see that ProcessFinalRefs now takes 0.5ms as opposed to 25ms before as the number of objects in the finalizer queue is substantially down.

What do people think about having a MIXED SSL provider in Netty? We're obviously going to benefit from it, but I wasn't sure how other adopters find it useful.

UPDATE: Bryce's #9626 would make it possible to hack this on our end, within Finagle by defaulting to the ref-counted SSL provider and manually wrapping SslContext with a class that implements a finalizer.

@netty-bot
Copy link

Can one of the admins verify this patch?

@bryce-anderson
Copy link
Contributor

bryce-anderson commented Oct 1, 2019

This looks correct to me, but I'm also unsure about the question of if this is helpful for others. As Vladimir noted, we have a way forward to the same end if #9626 gets merged, but if others have the same issue maybe this could help more folks.

@vkostyukov
Copy link
Contributor Author

Abandoning this as #9626 is merged.

@vkostyukov vkostyukov closed this Oct 7, 2019
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

3 participants