[
https://issues.apache.org/jira/browse/IO-468?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14312133#comment-14312133
]
Bernd Hopp commented on IO-468:
-------------------------------
@Sebb
"think the benchmark is not really the main issue here, although it will be
interesting to know whether or not ThreadLocal is quicker than memory
allocation, and for what buffer sizes. Though of course that may change with
different JVMs."
If you think my test is flawed, fix it or try a different JVM and prove me
wrong. I will intoduce a validation in the test that validates that the
inputStream and outputStream contents are identical, thereby forcing the
runtime to not optimize the copy process away. I do not think that there will
be different results, but we'll see. However as it is now I see a __MASSIVE__
performance improvement that you just seem to deny.
"The point is that the patch has a side effect, which is that memory is held
for longer periods than may be necessary."
Did you read my last posts? I already changed the implementation to use
WeakReferences, so that enables the GC to collect the buffers any time, I
mentioned this yesterday. Since threadpools may be reused in application
servers, I admit that this may COULD HAVE BEEN a problem, although I still
consider it to be a corner case.
"Also, it's not that allocation of the buffer is particularly slow, so even if
ThreadLocal is twice as fast, it's not going to make much difference to the
average app"
Why do you claim that 'allocation of the buffer is not particularly slow', when
test data indicates that there is a 1500% increase in performance when avoiding
buffer allocation? As I said: if you don't trust my test, reproduce it or
change it and prove me wrong. But don't just deny the test results and repeat
claims that have shown to be completely bogus.
"But it may make some apps with multiple threads use much more memory."
As mentioned, the current implementation uses WeakReferences now to avoid any
memory leaks. Can we drop this issue now? Or do you have evidence supporting
your claim?
"If the patch speeded up the code for all conditions, and had no side effects,
that would be different."
So where is performance or memory consumption worse or where are the side
effects? Please show me test results or reproducable conditions for
side-effects. How many times again do I have to ask you for evidence for your
claims?
"Furthermore, an app that does rely heavily on the copy methods can provide the
appropriate buffers in order to gain a performance benefit.
But that will need to be tested in the context of the app itself to know
whether the trade-off between memory usage and allocation speed is worth it or
not."
This patch massively speeds up standard methods with no side effects AFAIK. If
you suggest I implement some switches to give the user more control over
buffersize etc., I could do that. Otherwise, I don't see the problem.
> Avoid allocating memory for method internal buffers, use threadlocal memory
> instead
> -----------------------------------------------------------------------------------
>
> Key: IO-468
> URL: https://issues.apache.org/jira/browse/IO-468
> Project: Commons IO
> Issue Type: Improvement
> Components: Utilities
> Affects Versions: 2.4
> Environment: all environments
> Reporter: Bernd Hopp
> Priority: Minor
> Labels: newbie, performance
> Fix For: 2.5
>
> Attachments: PerfTest.java, monitoring_with_threadlocals.png,
> monitoring_without_threadlocals.png, performancetest.ods,
> performancetest_weakreference.ods
>
> Original Estimate: 12h
> Remaining Estimate: 12h
>
> In a lot of places, we allocate new buffers dynamically via new byte[]. This
> is a performance drawback since many of these allocations could be avoided if
> we would use threadlocal buffers that can be reused. For example, consider
> the following code from IOUtils.java, ln 2177:
> return copyLarge(input, output, inputOffset, length, new
> byte[DEFAULT_BUFFER_SIZE]);
> This code allocates new memory for every copy-process, that is not used
> outside of the method and could easily and safely reused, as long as is is
> thread-local. So instead of allocating new memory, a new utility-class could
> provide a thread-local bytearray like this:
> byte[] buffer = ThreadLocalByteArray.ofSize(DEFAULT_BUFFER_SIZE);
> return copyLarge(input, output, inputOffset, length, buffer);
> I have not measured the performance-benefits yet, but I would expect them to
> be significant, especially when the streams itself are not the performance
> bottleneck.
> Git PR is at https://github.com/apache/commons-io/pull/6/files
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
