Skip to content

Conversation

JarvisCraft
Copy link
Contributor

@JarvisCraft JarvisCraft commented Aug 30, 2024

This is a small change which utlizes a more domain-specific and potentially performant computeIfAbsent in MarkerManager.

Checklist

  • Base your changes on 2.x branch if you are targeting Log4j 2; use main otherwise
  • ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
  • Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
  • Tests for the changes are provided There are no functional changes
  • Commits are signed (optional, but highly recommended)

Copy link

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

@ppkarwasz
Copy link
Contributor

@JarvisCraft,

LGTM, thanks.

@ppkarwasz ppkarwasz merged commit 23cae42 into apache:2.x Aug 30, 2024
9 checks passed
@vy
Copy link
Member

vy commented Aug 31, 2024

@JarvisCraft, thanks so much for the contribution! 🙇 Curious: How did you encounter the problem? Do you observe a performance issue? If so, mind sharing some more details, please?

@vy vy added enhancement Additions or updates to features api Affects the public API labels Aug 31, 2024
@JarvisCraft
Copy link
Contributor Author

JarvisCraft commented Aug 31, 2024

@vy, hi! I was just looking through the sources of MarkerManager (interested in its implementation details) and, when looking at this specific method, just saw the common pattern which is almost always replaceable with the computeIfAbsent.

While I did not need any practical performance improvement from it (our use case for Markers is initializing them once into a static final field) this generally should be better from microopt. point of view since Map implementation may (and commonly will) be able to perform single key lookup instead of multiple ones and, what's more important, ConcurrentHashMap explicitly guarantees its atomicity.

But what's also important IMO is that the code is now more explicit now in what it does.

@ppkarwasz ppkarwasz added this to the 2.24.0 milestone Aug 31, 2024
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.

3 participants