Hi Gary,
On 17.10.2025 16:49, [email protected] wrote: > - static final class ScratchBufferHolder { > + static final class ScratchBytes implements AutoCloseable { > > /** > - * Holder for internal byte array buffer. > + * Wraps an internal byte array. > + * > + * [0] boolean in use. > + * [1] byte[] buffer. > */ > - private static final ThreadLocal<Object[]> > SCRATCH_BYTE_BUFFER_HOLDER = ThreadLocal.withInitial(() -> new Object[] { > false, byteArray() }); > - > - /** > - * Holder for internal char array buffer. > - */ > - private static final ThreadLocal<Object[]> > SCRATCH_CHAR_BUFFER_HOLDER = ThreadLocal.withInitial(() -> new Object[] { > false, charArray() }); > - > + private static final ThreadLocal<Object[]> LOCAL = > ThreadLocal.withInitial(() -> new Object[] { false, new > ScratchBytes(byteArray()) }); > > /** > * Gets the internal byte array buffer. > * > * @return the internal byte array buffer. > */ > - static byte[] getScratchByteArray() { > - final Object[] holder = SCRATCH_BYTE_BUFFER_HOLDER.get(); > + static ScratchBytes get() { > + final Object[] holder = LOCAL.get(); > // If already in use, return a new array > if ((boolean) holder[0]) { > - return byteArray(); > + return new ScratchBytes(byteArray()); > } > holder[0] = true; > - return (byte[]) holder[1]; > + return (ScratchBytes) holder[1]; > + } I like the idea of splitting ScratchBufferHolder into ScratchBytes and ScratchChars, but storing custom classes inside a ThreadLocal can cause classloader memory leaks in application servers during redeployments. I took that into account in PR #801 when introducing ScratchBufferHolder, which is why I intentionally did not use AutoCloseable at the time. I followed up on your change and submitted PR #804 [2], which: - Removes the classloader leak risk, - Preserves your refactoring and usability improvements. However, I think we should discuss whether AutoCloseable is worth keeping: Option 1: Merge PR #804 - Pros: Preserves your usability improvements - Cons: Adds a second ThreadLocal lookup per access Option 2: Revert the commit introducing AutoCloseable - Pros: Restores original single lookup performance - Cons: Slightly more verbose usage Given that ScratchBytes/ScratchChars are used on hot paths, this trade-off matters. I would also like to kindly ask that changes related to a PR are discussed in the PR rather than pushed directly to master afterward. I know we differ on CTR vs. RTC, but keeping changes self-contained helps avoid a chain of post-merge fixes and makes review easier for everyone. The goal of a PR is to prevent this kind of back-and-forth after merging. Thanks, Piotr References: [1] https://github.com/apache/commons-io/pull/801 [2] https://github.com/apache/commons-io/pull/804 --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
