Revert "Use private Thrift members (#6430)"

[dlmarion]

GarbageCollectorIT.gcLotsOfCandidatesIT has consistently failed on a server since #6430 was merged. The failure happens when the garbage collector process is started with 32 MB of memory. This PR branch exists so that I can test this on that server.

This reverts commit bdcd798.

[dlmarion]

I think I found a real issue with #6430 and we might want to actually revert it. The original code would access a ByteBuffer directly. #6430 made those member variables private and the methods for accessing the ByteBuffer's make a copy before returning them.

[dlmarion]

Here's a good example of the impact of this change.

accumulo/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java

Lines 957 to 960 in 1234b63

	ArrayList<Entry<Key,Value>> entries = new ArrayList<>(scanResult.getResults().size());
	for (TKeyValue kv : scanResult.getResults()) {
	entries.add(new SimpleImmutableEntry<>(new Key(kv.getKey()), new Value(kv.getValue())));
	}

The current version of Key(TKey) looks like:

accumulo/core/src/main/java/org/apache/accumulo/core/data/Key.java

Lines 687 to 707 in 1234b63

	public Key(TKey tkey) {
	this.row = ByteBufferUtil.toBytes(tkey.bufferForRow());
	this.colFamily = ByteBufferUtil.toBytes(tkey.bufferForColFamily());
	this.colQualifier = ByteBufferUtil.toBytes(tkey.bufferForColQualifier());
	this.colVisibility = ByteBufferUtil.toBytes(tkey.bufferForColVisibility());
	this.timestamp = tkey.getTimestamp();
	this.deleted = false;
	if (row == null) {
	throw new IllegalArgumentException("null row");
	}
	if (colFamily == null) {
	throw new IllegalArgumentException("null column family");
	}
	if (colQualifier == null) {
	throw new IllegalArgumentException("null column qualifier");
	}
	if (colVisibility == null) {
	throw new IllegalArgumentException("null column visibility");
	}
	}

The same code prior to #6430 looks like:

accumulo/core/src/main/java/org/apache/accumulo/core/data/Key.java

Lines 688 to 707 in 1852dbf

	public Key(TKey tkey) {
	this.row = toBytes(tkey.row);
	this.colFamily = toBytes(tkey.colFamily);
	this.colQualifier = toBytes(tkey.colQualifier);
	this.colVisibility = toBytes(tkey.colVisibility);
	this.timestamp = tkey.timestamp;
	this.deleted = false;
	if (row == null) {
	throw new IllegalArgumentException("null row");
	}
	if (colFamily == null) {
	throw new IllegalArgumentException("null column family");
	}
	if (colQualifier == null) {
	throw new IllegalArgumentException("null column qualifier");
	}
	if (colVisibility == null) {
	throw new IllegalArgumentException("null column visibility");
	}

The calls to bufferForX create a copy of the ByteBuffer.

[ctubbsii]

I'm currently looking into alternatives. I think the private thrift members is a nice change, but I also recognize the problem with the extra protective copy. Please hold on merging this for now, while I investigate.

[ctubbsii]

I created #6457 as an alternative to reverting all this. It will remove the unwanted protective copies that are likely causing the performance problem.

[ctubbsii]

I'd prefer to resolve the issue with #6457 instead, if works.

[dlmarion]

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.

[ctubbsii]

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

[ctubbsii]

My next attempt to address the issue is #6459

[dlmarion]

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.

I don't think that's correct. It appears to me that the code currently in main performs a copy for both calls to getX and bufferForX. Here's what I'm seeing using TKey as an example:

accumulo/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKey.java

Lines 194 to 201 in 618addf

	public byte[] getRow() {
	setRow(org.apache.thrift.TBaseHelper.rightSize(row));
	return row == null ? null : row.array();
	}
	public java.nio.ByteBuffer bufferForRow() {
	return org.apache.thrift.TBaseHelper.copyBinary(row);
	}

The call to bufferForRow calls TBaseHelper.copyBinary. The call to getRow calls TBaseHelper.rightSize, then calls setRow(below) which also calls TBaseHelper.copyBinary.

accumulo/core/src/main/thrift-gen-java/org/apache/accumulo/core/dataImpl/thrift/TKey.java

Lines 208 to 211 in 618addf

	public TKey setRow(@org.apache.thrift.annotation.Nullable java.nio.ByteBuffer row) {
	this.row = org.apache.thrift.TBaseHelper.copyBinary(row);
	return this;
	}

Your change in #6457 to use the unsafe_binaries Thrift option removed the copy from the bufferForX methods but not the getX methods.

[dlmarion]

https://issues.apache.org/jira/browse/THRIFT-4555 is the original issue where unsafe_binaries was added to Thrift.

[ctubbsii]

I was focused on the behavior of the rightSize method and overlooked that the getRow method would call the setter, where it does another protective copy.

This is frustrating. The setter should have a protective copy, but the getRow method shouldn't be calling it. It should be doing some internal setRow instead. The unsafe_binaries option is nice, but we really only should have unsafe access directly via the bufferFor methods; we still want protective copies on the setters and constructor. I think we'll need to make a fix upstream, and am in favor of reverting until it's fixed upstream.

The alternative to reverting at this point seems to either modify the code after it is generated, to avoid calling the setter in the getter, or to strip out the protective copy only from the bufferFor methods without using the unsafe_binaries option, or to use the unsafe_binaries option and make sure we're doing our own protective copies in calling code everywhere we call the setter or constructor. None of those seem like good options, so hopefully it's not so bad to push a fix upstream.

[ctubbsii]

I updated #6459 to work around the apparent bug in Thrift that calls the public API setter and making an unnecessary copy when the array is requested from the getter.