-
Notifications
You must be signed in to change notification settings - Fork 566
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
Minimally-invasive refinement to start-up time calculation #1100
Conversation
server.start() | ||
.whenComplete((webServer, throwable) -> { | ||
if (null != throwable) { | ||
STARTUP_LOGGER.log(Level.FINEST, "Startup failed", throwable); | ||
throwRef.set(throwable); | ||
} else { | ||
long t = TimeUnit.MILLISECONDS.convert(System.nanoTime() - beforeT, TimeUnit.NANOSECONDS); | ||
long t = TimeUnit.MILLISECONDS.convert(System.nanoTime() - STARTUP_TIME, TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it technically possible for a server to be started, stopped and started again? I think we may need to redefine what we call startup time here. It should really be the elapsed time of calling server.start()
, trying to calculate anything else in this method would be inaccurate; any such calculation needs to happen elsewhere IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd originally noticed that, after some code was added to the start
method, the point where the start time was captured fell after that added work. In the slack discussion Tomas expressed the opinion that the reported time should include as much work as possible -- he mentioned CDI + the server start time.
Yes, it is conceivable that a server would be started, stopped, and restarted again although that's not the usual use case. Recording the system time at the very beginning of the start
method results in a reported start-up time of 100 to 300 ms (in informal experiments). That's significantly under the log message timestamp delta, and that kind of mismatch is what prompted this issue originally.
The CDI initialization occurs in Server.Builder.build()
(unless the caller to Builder
passes a previously-init'd CDI container which is not our usual case).
We could separately record the "initialization" time (which would include the CDI initialization) and the server start-up time. During the first start-up the log message could report the overall time and the two components; during a subsequent server restart it could report only the time in ServerImpl.start
because the other initialization is not repeated in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, because the server start-up time itself seems to be a relatively small part of the overall initialization time, we could report the overall initialization time only once, at the end of the first invocation of start
. Rereading what I wrote above it might be overly confusing to users to try to concisely convey the two separate times.
microprofile/server/src/main/java/io/helidon/microprofile/server/Server.java
Show resolved
Hide resolved
…, rather than 'startup'; report init timing only during the first invocation of the server's start method (since init code outside the server start accounts for by far the most time)
The latest commits address, I think, Santiago's comments while reporting the most useful numbers for the by-far most common use case. The log message now reports server initialization time rather than start-up time given that it encompasses not just server (newlines inserted manually for readability in the comment)
Also, the message is logged only during the first invocation of the |
Addresses #1099
The end-of-startup log message understated the actual start-up time for the MP server.
This PR simply captures the current system time at class-load-time of
ServerImpl
(which we now also trigger as early as possible inServer.Builder
). This does not include all of the JVM and class loading time but it greatly improves the accuracy of the start-up log message. For example:The reported start-up time in ms much more closely aligns with the log message timestamps.
Another option (I looked into this and @batsatt also mentioned it in slack) would have been to use the
RuntimeMXBean
which reports the JVM start-up time. In some informal measurements I was seeing a 40-50 ms cost for that.Yet another option (thanks to @spericas) would have collected the start-up time in the built-in
Main
class, but users can provide their ownMain
which would skip that (as Santiago also mentioned).The changes in the PR give a timing that matches up with the log message timestamps and also gives a reasonable approximation to the full start-up time. The
Server.Builder
andServerImpl
classes are loaded early during start-up.