ctubbsii commented on PR #6455: URL: https://github.com/apache/accumulo/pull/6455#issuecomment-4860721405
> GarbageCollectorIT is now passing again with these changes. Will probably end up closing this in favor of #6457 though if that also fixes the issue. I saw it fail due to the same timeout on the first pass, and then pass on the second attempt. I think that test is flaky already, and a revert will not entirely fix the issue. It's possible the changes in #6430 contributed to a performance issue, but I do not think the performance of that particular test is a reliable enough metric to determine whether #6430 is a good change or not. Even though I am reluctant to revert #6430, for fear of reversing progress, I do think there's more that probably needs to be done here. As it turns out, the getters that return a byte array is not even the code path that creates redundant protective copies. It's the versions of the methods that look like `.bufferForValue()` that are the ones that are doing unnecessary copies. The method that returns a byte array only makes a copy when it needs to truncate a byte buffer's backing array to size, and that is done at most once (and it's also probably important to do that, because the original backing array is a buffer in the TFramedTransport, and I'm not entirely convinced that it isn't reusable and unsafe as a backing array at this layer; I'd need to dig more into the deserialization code to understand that better). So really, we should be avoiding the `.bufferForValue()` style methods completely. In the vast majority of cases, we actually want the byte array anyway, and were just doing our own array extraction from the ByteBuffer. But we don't even need to do that if we use the correct getter that returns an array. Previously, I was thinking that the problem might be that we need to use the getter that returns the ByteBuffer more... but I now understand that is the opposite of what we need to do. I have abandoned #6457 -- 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]
