Batch refactoring #72
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
They're not necessarily tied to core and can/should run standalone, so they're better placed outside of core
This reduces the I/O part, which, for most tests, is something we'd want as it allows us to focus on the part of the code that really matters.
If the same thread is reusing the same `LogEvent`, we are able to skip resetting the object's `threadName` as it would be the same value anyway. This saves on GC and is specially useful for non-virtual threads environment.
Due to the fact we're "caching" the thread name, it is interesting to avoid getting new objects every time. While not making an incredible effort to save it, by using the thread's object hashCode, we're able to reduce clashes and capitalize better on the cache.
Most of those @OverRide methods were unnecessary duplication. By moving them out, we reduce the object sizes and reduce the overhead. The default case is `Info`, so `InfoGuard` doesn't have to override any methods. Higher or lower log level will only override the diff from the default case.
Furthermore, we open up the scope a bit as we're particularly interested in the `WritableByteChannel` interface only, so `CoreSink` and `DirectJson` doesn't need to rely on `FileChannel`.
The internal, record-based `KeyValuePair` should be preferred. In the future, if we decide to detach from `SLF4J`, it is best that the internals do not rely on `SLF4J` internals.
The tree-based implementation is very fast to update but not much to create new loggers. Furthermore, it might not be space efficient, though I haven't checked that thoroughly. One final word about it is that it could also be that fast because it is not necessarily thread safe. This implementation is more adequate because it is faster for creating a new logger than the tree-based version. It is likely more space efficient than the previous one as it doesn't have much acessory code around it.
It is more performant and saves more memory.
For that we need to make the LogConfig back a normal class, but this allows us to reuse data in there without having to recreate new instances of the same data (or data that is partially the same, for example, the StackTraceFilter, which is likely the same if we're only changing the logger level).
This comes with a surprising and very positive change. Now, it is not necessary to break the logger by `.` using a regex as the internal code of the storage handles that (without a regex), so we're left with better APIs and a smaller footprint for the LoggerFactory. Also, given we lifted the requirement of `String[]` in the config update code, we don't need to do any special-casing for the records, so they're now able to use the native methods, which makes the code much cleaner to maintain now.
This has proven to be much faster (~3x faster)
Makes the API more flexible and still behaves the same
Altough it is good practice to copy the array, we won't do that as we're sure upstream will never change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I know it is not nice to do a batch refactoring, but pulling this thread led to many changes, most notably:
coretests;