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

Remove properties cache and token-based matching #2849

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

ppkarwasz
Copy link
Contributor

The PropertiesUtil class preemptively caches lookups for all known keys from enumerable property sources.
On my Debian system a JVM has around 60 among environment variables and system properties, which causes PropertiesUtil to perform around 60 lookups for each of the 3 standard property sources. Most of these properties have nothing to do with Log4j.

On the other hand all Log4j artifacts use no more than 100 configuration properties and the results of most of those calls are cache in static fields.

This PR removes the property value caches used by PropertiesUtil.
The change should have no noticeable impact on the loading time of Log4j Core, while at the same time simplifies the system.

As remarkable side effect token-based matching of property names is no longer supported. This mean that, e.g. a system property like log4j2.asyncLoggerRingBufferSize can be written:

  • as AsyncLogger.RingBufferSize (pre-2.10 legacy form),
  • as log4j2.asyncLoggerRingBufferSize (2.10 canonical form).

No other forms (e.g. log4j.async.logger.ringBuffer.size) are accepted.

Note: We never documented that we support token-based matching. See current Configuration Properties documentation.

@ppkarwasz ppkarwasz self-assigned this Aug 18, 2024
@vy vy added enhancement Additions or updates to features api Affects the public API labels Aug 26, 2024
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change should have no noticeable impact on the loading time of Log4j Core, while at the same time simplifies the system.

Reminder: If the goal is simplification, we can even replace all existing sort-on-add lines with a single sort-while-iterating.

As remarkable side effect token-based matching of property names is no longer supported. This mean that, e.g. a system property like log4j2.asyncLoggerRingBufferSize can be written:

  • as AsyncLogger.RingBufferSize (pre-2.10 legacy form),
  • as log4j2.asyncLoggerRingBufferSize (2.10 canonical form).

No other forms (e.g. log4j.async.logger.ringBuffer.size) are accepted.

Note: We never documented that we support token-based matching.

I would have expected following properties (collected from the rel/2.23.1 site) to not work anymore:

  • log4j2.enable.direct.encoders
  • log4j2.jmx.notify.async

Though they still do work. Would you mind explaining why, please?

@ppkarwasz
Copy link
Contributor Author

The change should have no noticeable impact on the loading time of Log4j Core, while at the same time simplifies the system.

Reminder: If the goal is simplification, we can even replace all existing sort-on-add lines with a single sort-while-iterating.

I dropped a comment in there. Replacing the locks and the ArrayList with a concurrent set introduces only a couple of µs to the startup process, so should be negligible.

I applied the suggested change in 28b46de.

I would have expected following properties (collected from the rel/2.23.1 site) to not work anymore:

* `log4j2.enable.direct.encoders`

* `log4j2.jmx.notify.async`

Though they still do work. Would you mind explaining why, please?

These are the correct pre-2.10 forms of these properties, that is why they work.

I don't plan for this change to be so drastic as to disable configuration properties that were documented in previous Log4j Core versions. I only want to remove those that were not documented, but worked by some chance, e.g. log4j2.jmx.notifyAsync.

@ppkarwasz ppkarwasz requested a review from vy August 26, 2024 09:54
@ppkarwasz
Copy link
Contributor Author

@vy,

I did non-trivial changes in 28b46de. Can you review this again?

@ppkarwasz ppkarwasz added this to the 2.24.0 milestone Aug 26, 2024
The `PropertiesUtil` class preemptively caches lookups for all known keys from enumerable property sources.
On my Debian system a JVM has around 60 among environment variables and system properties, which causes `PropertiesUtil` to perform around 60 lookups for **each** of the 3 standard property sources. Most of these properties have nothing to do with Log4j.

On the other hand all Log4j artifacts use no more than 100 configuration properties and the results of most of those calls are cache in static fields.

This PR removes the property value caches used by `PropertiesUtil`.
The change should have no noticeable impact on the loading time of Log4j Core, while at the same time simplifies the system.
@ppkarwasz ppkarwasz force-pushed the feature/2.x/remove-properties-cache branch from 28b46de to 78af316 Compare August 26, 2024 11:51
@ppkarwasz ppkarwasz merged commit 78af316 into 2.x Aug 26, 2024
7 checks passed
@ppkarwasz ppkarwasz deleted the feature/2.x/remove-properties-cache branch August 26, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API enhancement Additions or updates to features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants