kilink commented on PR #3868: URL: https://github.com/apache/logging-log4j2/pull/3868#issuecomment-3160865030
I personally don't think it's worthwhile to spend time writing a benchmark for what amounts to a bug fix / correctness issue. If you'd rather not accept the change, feel free to close the issue. I'll leave my rationale for fixing it however: - Without a benchmark, we can see that the incorrect supplied capacity will cause a resize / rehash, and result in a map that is over-sized for the number of items inserted. It's easy to avoid with a trivial calculation which takes into account the load factor. - It's true that not all instances of this I changed in the PR are likely to be on the hot path, but fixing it everywhere is more of a consistency thing; if the helper is there, why not, otherwise it would just be more confusing. - There are likely callers in the wild that at least would be hitting the `toMap` implementation of the `ReadOnlyStringMap`, e.g. likely through `ThreadContextMap` (I can confirm such instances internally, as well as in OSS projects such as Sentry). - Whoever wrote those lines of code was attempting to size the HashMaps precisely by passing in a capacity, it just so happens that the number of items to insert != the map capacity, so seems like an easy mistake to have made. I assume care was taken here though since log4j generally tries to be optimal in terms of allocations and performance. I assume the main point of contention is the addition of a utility class, and I agree it is ugly, but it's a stopgap until (?) Log4j can use the static `HashMap.newHashMap` added in Java 19, I presume when the baseline JDK version log4j requires allows us to use that, we can just switch to it and remove any such shim utility class. Or another way to put it, I presume you would readily accept this change if it were simply delegating to the JDK helpers (assuming we could use it)? -- 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]
