ppkarwasz commented on PR #3868: URL: https://github.com/apache/logging-log4j2/pull/3868#issuecomment-3158723587
Hi @kilink, Adding a new utility class to `log4j-api` and then using it in `log4j-core` reinforces an unnatural dependency between the two modules, and would introduce a hard incompatibility between Log4j Core `2.26.0` and Log4j API `2.25.x`. The Core module should implement the API, not depend on it for internal utilities. I realize this kind of coupling already exists in some places, but I’d prefer not to increase it. Could you instead: * Move `Maps` to `o.a.l.l.util.internal` to make it clear it’s not intended for external use. While `o.a.l.l.util` is *theoretically* internal, it remains exported due to past API leaks (e.g., `o.a.l.l.util.Supplier`). * Create a separate `o.a.l.l.core.util.internal.Maps` helper for `log4j-core`, so it can be used independently without introducing additional cross-module dependencies. I agree with @vy that, without clear benchmark results, the value of this enhancement is questionable. We avoid using `HashMap` in performance-critical paths (such as the logging context map) because its instantiation and operation costs are high for small maps. Instead, we use `o.a.l.l.util.StringMap`, which offers enhanced features like mutability control (`freeze()`) and a `foreach` method (introduced for `Map` only in JDK 8). However, `StringMap` represents a significant technical debt we would like to eliminate. If you are interested in map performance, could you explore the feasibility of replacing `StringMap` with `Map` in version 3.x? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
