Set useOpenSsl in Flags lazily to reduce server startup time #2184

mumgmangmange · 2019-10-12T00:18:00Z

Motivation:
Related #1645
useOpenSsl in Flags is used only when TLS is enabled. However, it takes a lot to instantiate OpenSsl class so it adds up a tremendous time to the server startup time. We should fix to instantiate it only when it needs.

Modification:

Set useOpenSsl in Flags lazily
Check whether the OS is Linux before calling Epoll.isAvailable()
Add OsType

Result:

Reduced server startup time to 80 percent (20% decreased)

minwoox

Way to go @mumgmangmange!
Let's initiate USE_EPOLL lazily as we do for USE_OPENSSL.
Also, if you let us know how much the server startup time decreases, that'll be much appreciated.

core/src/main/java/com/linecorp/armeria/common/Flags.java

minwoox · 2019-10-14T10:59:19Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

-
-    private static final boolean USE_OPENSSL = getBoolean("useOpenSsl", OpenSsl.isAvailable(),
-                                                          value -> OpenSsl.isAvailable() || !value);
+    private static Boolean USE_OPENSSL = null;


I think we don't need to synchronize to set this value but I think we still need to make this volatile because some of the threads might not see this. /cc @trustin, @ikhoon

Because this is not a static final constant anymore, we should use camel case.

Could you leave a comment about why we initialize this field lazily?

Changed to Camel. Thank you.

Do not call OpenSsl.isAvailable() when not using SSL.
0.2 to 0.3 seconds shortened.

could this change be in a patch release with boot 2.2? If it is that much overhead, this will reduce zipkin-slim startup by 15% (I can verify). We are nearing a redo of our release, and it would make the announcement much better I think.

cc @trustin @anuraaga

core/src/main/java/com/linecorp/armeria/common/Flags.java

trustin · 2019-10-17T06:38:03Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

        // Netty epoll transport does not work with WSL (Windows Sybsystem for Linux) yet.
        // TODO(trustin): Re-enable on WSL if https://github.com/Microsoft/WSL/issues/1982 is resolved.


Please move this comment inside the if block below for readability.

I changed. Thank you. 😄

trustin · 2019-10-17T06:43:08Z

There are 3 Checkstyle violations:

[ant:checkstyle] [ERROR] /appveyor/projects/armeria/core/src/main/java/com/linecorp/armeria/common/Flags.java:120:28: Variable 'USE_OPENSSL' explicitly initialized to 'null' (default value for its type). [ExplicitInitialization]
[ant:checkstyle] [ERROR] /appveyor/projects/armeria/core/src/main/java/com/linecorp/armeria/common/Flags.java:412: Line is longer than 112 characters (found 116). [LineLength]
[ant:checkstyle] [ERROR] /appveyor/projects/armeria/core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java:84:8: Unused import - io.netty.handler.ssl.OpenSsl. [UnusedImports]

Just in case it's unclear, the first violation means you do not need to assign null to a variable, because a variable's default value is null already. The other two look obvious, right?

CLAassistant · 2019-10-17T13:15:03Z

All committers have signed the CLA.

core/src/main/java/com/linecorp/armeria/common/Flags.java

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

anuraaga · 2019-10-19T07:21:19Z

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

+    /**
+     * Return whether Linux is used.
+     */
+    public static boolean isLinux() {
        return Ascii.toLowerCase(System.getProperty("os.name", "")).startsWith("linux");


Now that this is a public method, we should go ahead and optimize the method. Can you add private static final boolean IS_LINUX = Ascii.toLowerCase(System.getProperty("os.name", "")).startsWith("linux"); at the beginning and return that here?

Also, anyone have thoughts on making this public means we should instead return an enum of LINUX, WINDOWS, MAC_OS_X, OTHER?

Yes, how about replacing isLinux() with the following method?

private static final OsType OS_TYPE; static { // ... Detect OS type here ... OS_TYPE = ...; } public static OsType osType() { return OS_TYPE; }

OsType could look like:

public enum OsType { LINUX, WINDOWS, // MAC_OS_X, FREEBSD, ... (Do we need them?) OTHERS }

OK 😄 I understand.
I modified with reference to comment.

trustin

It getting a little bit more effort than expected but I guess it will be useful to have SystemInfo.osType(). PTAL, @mumgmangmange.

trustin · 2019-10-19T11:09:31Z

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

+    /**
+     * Return whether Linux is used.
+     */
+    public static boolean isLinux() {
        return Ascii.toLowerCase(System.getProperty("os.name", "")).startsWith("linux");


Yes, how about replacing isLinux() with the following method?

private static final OsType OS_TYPE; static { // ... Detect OS type here ... OS_TYPE = ...; } public static OsType osType() { return OS_TYPE; }

OsType could look like:

public enum OsType { LINUX, WINDOWS, // MAC_OS_X, FREEBSD, ... (Do we need them?) OTHERS }

trustin

Oops, meant to request changes. Sorry. 😅

codefromthecrypt

maybe too much for this PR, but I would love to see a "Platform" like type that does the detection of things like this in a lazy way.

https://github.com/square/okhttp/blob/master/okhttp/src/main/java/okhttp3/internal/platform/Platform.kt#L199

If single dimension doesn't cover parts we need..
Platform.get().isOpenSslAvailable()

We could instead do it like this..
Platform.get().os().isOpenSslAvailable() where the .os() looks deeper at the OS type and also caches it.

Don't let that slow this work, though (pun intended).

codefromthecrypt · 2019-10-21T03:52:48Z

fyi this inspired me to notice and fix some things in brave. thanks! openzipkin/brave#1006

anuraaga · 2019-10-21T05:04:53Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

+        if (useOpenSsl != null) {
+            return useOpenSsl;
+        }
+        useOpenSsl = getBoolean("useOpenSsl", OpenSsl.isAvailable(), value -> OpenSsl.isAvailable() || !value);


I think we can avoid isAvailable if the user explicitly disabled OpenSSL.

final boolean useOpenSsl = getBoolean("useOpenSsl", true); if (!useOpenSsl) { // OpenSSL explicitly disabled return Flags.useOpenSsl = false; } if (!OpenSsl.isAvailable(){ ... } else { ... }

Is this useOpenSsl variable unconditionally true?

In this case, true is the default value unless a user sets com.linecorp.armeria.useOpenSsl=false.

At least I think it is let me know if I missed something :)

Yes. I understand 😃

anuraaga · 2019-10-21T05:05:56Z

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

@@ -109,6 +111,12 @@
                JETTY_ALPN_OPTIONAL_OR_AVAILABLE = false;
            }
        }
+
+        try {
+            osType = OsType.valueOf(Ascii.toUpperCase(System.getProperty("os.name", "").split(" ")[0]));


OS detection is a bit more complicated unfortunately.

Can you use the if statements for the OsType OS's from this famous OS detection logic?

https://github.com/trustin/os-maven-plugin/blob/master/src/main/java/kr/motd/maven/os/Detector.java#L150

OK. 😃 Changed to an if statement.

anuraaga · 2019-10-21T05:06:38Z

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

+        WINDOWS,
+        LINUX,
+        MAC,
+        FREEBSD,


I would lean towards leaving out FREEBSD and SOLARIS - it's unlikely Armeria servers would have logic specific to them anyways so they can go in OTHERS

OK. 😄
I thought FREEBSD and SOLARIS as OTHERS and changed it.

minwoox

Just left some nits. 😄

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

anuraaga · 2019-10-23T03:02:00Z

core/src/main/java/com/linecorp/armeria/common/Flags.java

@@ -422,7 +403,30 @@ public static boolean useEpoll() {
     * {@code -Dcom.linecorp.armeria.useOpenSsl=false} JVM option to disable it.
     */
    public static boolean useOpenSsl() {
-        return USE_OPENSSL;


I think we still need the null check that returns the value after we set it once like we had before (can use a different name for the local variable if it makes it confusing).

core/src/main/java/com/linecorp/armeria/common/Flags.java

anuraaga

Thanks!

minwoox

Thanks a lot @mumgmangmange! 👍

core/src/main/java/com/linecorp/armeria/common/Flags.java

trustin

Could you move OsType to top level? LGTM except that.

ikhoon

@mumgmangmange Thanks for your long efforts! LGTM please address @trustin review.

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java

)

…ing system is Linux. (#2184)

minwoox · 2019-10-25T03:06:48Z

@anuraaga, any other comments?

codecov · 2019-10-25T03:51:41Z

Codecov Report

Merging #2184 into master will increase coverage by 0.05%.
The diff coverage is 32.5%.

@@             Coverage Diff             @@
##             master   #2184      +/-   ##
===========================================
+ Coverage     73.64%   73.7%   +0.05%     
- Complexity     9620    9635      +15     
===========================================
  Files           839     840       +1     
  Lines         37022   37041      +19     
  Branches       4561    4566       +5     
===========================================
+ Hits          27266   27302      +36     
+ Misses         7432    7408      -24     
- Partials       2324    2331       +7

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/linecorp/armeria/server/ServerBuilder.java	`74.54% <0%> (ø)`	`106 <0> (ø)`	⬇️
.../java/com/linecorp/armeria/common/util/OsType.java	`100% <100%> (ø)`	`1 <1> (?)`
...a/com/linecorp/armeria/common/util/SystemInfo.java	`25.46% <20%> (-0.85%)`	`6 <1> (ø)`
...c/main/java/com/linecorp/armeria/common/Flags.java	`61.03% <25%> (-0.64%)`	`54 <3> (+1)`
...inecorp/armeria/server/grpc/ArmeriaServerCall.java	`87.1% <0%> (-0.4%)`	`85% <0%> (-1%)`
...ecorp/armeria/spring/ArmeriaAutoConfiguration.java	`80.43% <0%> (+0.43%)`	`11% <0%> (+1%)`	⬆️
...a/common/grpc/protocol/ArmeriaMessageDeframer.java	`72.81% <0%> (+0.48%)`	`49% <0%> (+1%)`	⬆️
...com/linecorp/armeria/server/HttpServerHandler.java	`82.05% <0%> (+0.66%)`	`83% <0%> (+1%)`	⬆️
.../linecorp/armeria/client/Http2ResponseDecoder.java	`61.66% <0%> (+0.83%)`	`33% <0%> (+1%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 610e29e...8bac05c. Read the comment docs.

minwoox · 2019-10-25T05:27:27Z

@mumgmangmange Could you sign the CLA so that we can merge this PR?

minwoox · 2019-10-25T05:36:40Z

congratulations on your first PR, @mumgmangmange!
Thank you~! and Thanks, reviewers!

Motivation: Related line#1645 `useOpenSsl` in `Flags` is used only when TLS is enabled. However, it takes a lot to instantiate `OpenSsl` class so it adds up a tremendous time to the server startup time. We should fix to instantiate it only when it needs. Modification: - Set `useOpenSsl` in `Flags` lazily - Check whether the OS is Linux before calling `Epoll.isAvailable()` - Add `OsType` Result: - Reduced server startup time to 80 percent (20% decreased)

mumgmangmange requested review from ikhoon, minwoox and trustin as code owners October 12, 2019 00:18

minwoox mentioned this pull request Oct 14, 2019

Issue1645 #2156

Closed

minwoox changed the title ~~Issue 164501~~ Set USE_OPENSSL and USE_EPOLL in Flags when they need Oct 14, 2019

minwoox reviewed Oct 14, 2019

View reviewed changes

minwoox added the improvement label Oct 14, 2019

minwoox added this to the 0.95.0 milestone Oct 14, 2019

minwoox changed the title ~~Set USE_OPENSSL and USE_EPOLL in Flags when they need~~ Set useOpenSsl in Flags lazily to reduce server startup time Oct 15, 2019

ikhoon requested changes Oct 15, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

trustin requested changes Oct 17, 2019

View reviewed changes

ikhoon requested changes Oct 18, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java Outdated Show resolved Hide resolved

anuraaga reviewed Oct 19, 2019

View reviewed changes

trustin approved these changes Oct 19, 2019

View reviewed changes

trustin requested changes Oct 19, 2019

View reviewed changes

codefromthecrypt reviewed Oct 21, 2019

View reviewed changes

anuraaga reviewed Oct 21, 2019

View reviewed changes

codefromthecrypt mentioned this pull request Oct 21, 2019

investigate virtualhost.defaultHostname #2202

Closed

minwoox reviewed Oct 22, 2019

View reviewed changes

ikhoon requested changes Oct 22, 2019

View reviewed changes

ikhoon reviewed Oct 23, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java Outdated Show resolved Hide resolved

anuraaga reviewed Oct 23, 2019

View reviewed changes

anuraaga approved these changes Oct 23, 2019

View reviewed changes

minwoox approved these changes Oct 24, 2019

View reviewed changes

anuraaga reviewed Oct 24, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/Flags.java Outdated Show resolved Hide resolved

trustin approved these changes Oct 24, 2019

View reviewed changes

ikhoon approved these changes Oct 24, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java Outdated Show resolved Hide resolved

anuraaga reviewed Oct 24, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/util/SystemInfo.java Outdated Show resolved Hide resolved

sohyunpark and others added 12 commits October 24, 2019 23:21

Modify the USE_OPENSSL variable to useOpenSsl. (#2184)

7a81859

Add comment : Return whether Linux is used. (#2184)

1d72915

Created SystemInfo.osType() (#2184)

733bb10

Classify FREEBSD and SOLARIS of OsType as OTHERS (#2184)

ea122ab

The OS detection logic has been changed to an if statement (#2184)

86d969a

Redefine osType Variable Value Allocation (#2184)

f17571a

Explicit handling of Linux usage (#2184)

58d6f3a

Set return value when user sets com.linecorp.armeria.useOpenSsl=? (#2184

33b46ab

)

javadoc of isLinux method changed: Returns {@code true} if the operat…

17392d9

…ing system is Linux. (#2184)

Override useOpenSsl Condition (#2184)

248a13b

Clean up

c191c1e

Address comments by @anuraaga & fix checkstyle

8bac05c

anuraaga approved these changes Oct 25, 2019

View reviewed changes

minwoox merged commit 5eb5ac3 into line:master Oct 25, 2019

minwoox mentioned this pull request Jan 20, 2020

Startup time optimization #1645

Closed

ikhoon mentioned this pull request Oct 18, 2023

Add TlsEngineType enum #5029

Merged

		// Netty epoll transport does not work with WSL (Windows Sybsystem for Linux) yet.
		// TODO(trustin): Re-enable on WSL if https://github.com/Microsoft/WSL/issues/1982 is resolved.

Set useOpenSsl in Flags lazily to reduce server startup time #2184

Set useOpenSsl in Flags lazily to reduce server startup time #2184

Conversation

mumgmangmange commented Oct 12, 2019 • edited by minwoox Loading

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin commented Oct 17, 2019

CLAassistant commented Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

codefromthecrypt left a comment

Choose a reason for hiding this comment

codefromthecrypt commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Oct 25, 2019

codecov bot commented Oct 25, 2019 • edited Loading

Codecov Report

minwoox commented Oct 25, 2019

minwoox commented Oct 25, 2019

mumgmangmange commented Oct 12, 2019 •

edited by minwoox

Loading

CLAassistant commented Oct 17, 2019 •

edited

Loading

codecov bot commented Oct 25, 2019 •

edited

Loading