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

Suggesting improve initialization performance #753

Open
wants to merge 9 commits into
base: v2.8
Choose a base branch
from

Conversation

timlueg
Copy link

@timlueg timlueg commented Jul 7, 2024

Description

I was happy to see that there are logging libraries in java that don't need multiple 100s of milliseconds just for initialization.

Here I made some suggestions to reduces the tinylogs init performance further by ~10ms

  • Replace ClassContextSecurityManager() with StackWalker
  • Use direct call instead of reflection in getCurrentProcess (unsure why it uses reflection here)
  • Use System.currentTimeMillis() instead of JVM start time. Really unsure if this would be acceptable but this save about 5ms it think.

Agreements

  • I agree that my changes will be published under the terms of the Apache License 2.0 (mandatory)
  • I agree that my GitHub user name will be published in the release notes (optional)

@timlueg timlueg changed the title Suggesting improve Initialization performance Suggesting improve initialization performance Jul 7, 2024
Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.31%. Comparing base (7590ad1) to head (292e94c).

Files Patch % Lines
...in/java/org/tinylog/runtime/ModernJavaRuntime.java 95.23% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               v2.8     #753      +/-   ##
============================================
+ Coverage     94.25%   94.31%   +0.05%     
+ Complexity     2838     2835       -3     
============================================
  Files           149      149              
  Lines          5502     5505       +3     
  Branches        727      723       -4     
============================================
+ Hits           5186     5192       +6     
+ Misses          176      175       -1     
+ Partials        140      138       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pmwmedia
Copy link
Member

pmwmedia commented Jul 7, 2024

Thanks for the draft! I'm curious to observe your changes. Just let me know when you are done, so I can review the changes.

To answer your questions:

Use direct call instead of reflection in getCurrentProcess (unsure why it uses reflection here)

Reflections where used for compatibility with Android as Android doesn't provide the class ProcessHandle, which was a cause for issues. However, I don't think that this workaround is still required.

Use System.currentTimeMillis() instead of JVM start time. Really unsure if this would be acceptable but this save about 5ms it think.

The JVM start time is nice if tinylog will be initialized much later. However, if you can provide a benchmark that shows significant performance improvements, we can change this.

@timlueg
Copy link
Author

timlueg commented Jul 28, 2024

ManagementFactory.getRuntimeMXBean().getStartTime(); Takes 6-10 ms on my system (with openjdk-22.0.1)
Where as System.currentTimeMillis() takes <0.002 ms.

(It was tricky to get this project build locally (at least in Intellij), I experienced issues with JDK certificates, probably because of the old jdk version requirement. Don't want to spam more commits to get build in ci working)

@timlueg timlueg marked this pull request as ready for review July 28, 2024 16:35
@pmwmedia
Copy link
Member

You can fix the build by applying these changes to ModernJavaRuntime.java:

	@IgnoreJRERequirement
	private static final class ClassNameExtractorByDepth implements Function<Stream<StackFrame>, String> {
		private final int depth;

		private ClassNameExtractorByDepth(final int depth) {
			this.depth = depth;
		}

		@Override
		public String apply(final Stream<StackFrame> frames) {
			return frames.skip(depth)
					.findFirst()
					.map(new ClassNameMapper())
					.orElse(null);
		}
	}

	@IgnoreJRERequirement
	private static final class ClassNameExtractorByLoggerClassName implements Function<Stream<StackFrame>, String> {
		private final String loggerClassName;

		private ClassNameExtractorByLoggerClassName(final String loggerClassName) {
			this.loggerClassName = loggerClassName;
		}

		@Override
		public String apply(final Stream<StackFrame> stream) {
			return stream.map(new ClassNameMapper()).dropWhile(new Predicate<String>() {
				@Override
				public boolean test(final String name) {
					return !name.equals(loggerClassName);
				}
			}).skip(1).findFirst().orElse(null);
		}
	}

	/**
	 * Mapper for getting the class name from a {@link StackFrame}.
	 */
	@IgnoreJRERequirement
	private static final class ClassNameMapper implements Function<StackFrame, String> {

		/** */
		private ClassNameMapper() {
		}

		@Override
		public String apply(final StackFrame stackFrame) {
			return stackFrame.getClassName();
		}

	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants