[Jython-checkins] jython: Implementation of PyBuffer.copyFrom dealing correctly with overlap.
jeff.allen
jython-checkins at python.org
Sat Aug 27 09:12:25 EDT 2016
https://hg.python.org/jython/rev/ac7255b9f994
changeset: 7945:ac7255b9f994
user: Jeff Allen <ja.py at farowl.co.uk>
date: Sun Jun 26 22:04:50 2016 +0100
summary:
Implementation of PyBuffer.copyFrom dealing correctly with overlap.
Fixes regression in test_memoryview due to recent refactoring. JUnit
test provided explicitly for copyFrom() sliced versions of self.
files:
src/org/python/core/buffer/Base1DBuffer.java | 2 +-
src/org/python/core/buffer/BaseArrayBuffer.java | 72 ++--
src/org/python/core/buffer/BaseBuffer.java | 17 +-
src/org/python/core/buffer/BaseNIOBuffer.java | 55 ---
tests/java/org/python/core/PyBufferTest.java | 161 ++++++++-
5 files changed, 180 insertions(+), 127 deletions(-)
diff --git a/src/org/python/core/buffer/Base1DBuffer.java b/src/org/python/core/buffer/Base1DBuffer.java
--- a/src/org/python/core/buffer/Base1DBuffer.java
+++ b/src/org/python/core/buffer/Base1DBuffer.java
@@ -77,7 +77,7 @@
} else if (stride > 0) {
return index0 + (shape[0] - 1) * stride;
} else {
- return index0 - 1;
+ return index0;
}
}
diff --git a/src/org/python/core/buffer/BaseArrayBuffer.java b/src/org/python/core/buffer/BaseArrayBuffer.java
--- a/src/org/python/core/buffer/BaseArrayBuffer.java
+++ b/src/org/python/core/buffer/BaseArrayBuffer.java
@@ -113,6 +113,23 @@
@Override
public void copyFrom(byte[] src, int srcPos, int destIndex, int count)
throws IndexOutOfBoundsException, PyException {
+ copyFrom(src, srcPos, 1, destIndex, count);
+ }
+
+ /**
+ * Generalisation of {@link PyBuffer#copyFrom(byte[], int, int, int)} to allow a stride within
+ * the source array.
+ *
+ * @param src source byte array
+ * @param srcPos byte-index location in source of first byte to copy
+ * @param srcStride byte-index increment from one item to the next
+ * @param destIndex starting item-index in the destination (i.e. <code>this</code>)
+ * @param count number of items to copy in
+ * @throws IndexOutOfBoundsException if access out of bounds in source or destination
+ * @throws PyException (TypeError) if read-only buffer
+ */
+ protected void copyFrom(byte[] src, int srcPos, int srcStride, int destIndex, int count)
+ throws IndexOutOfBoundsException, PyException {
checkWritable();
@@ -123,16 +140,19 @@
int skip = stride - itemsize;
int d = byteIndex(destIndex);
+ int srcSkip = srcStride - itemsize;
+
// Strategy depends on whether items are laid end-to-end or there are gaps
- if (skip == 0) {
+ if (skip == 0 && srcSkip == 0) {
// Straight copy of contiguous bytes
System.arraycopy(src, srcPos, storage, d, count * itemsize);
} else {
- // Non-contiguous copy: single byte items
int limit = d + count * stride, s = srcPos;
if (itemsize == 1) {
+ // Non-contiguous copy: single byte items
for (; d != limit; d += stride) {
- storage[d] = src[s++];
+ storage[d] = src[s];
+ s += srcStride;
}
} else {
// Non-contiguous copy: itemsize bytes then skip to next item
@@ -141,6 +161,7 @@
while (d < t) {
storage[d++] = src[s++];
}
+ s += srcSkip;
}
}
}
@@ -149,56 +170,41 @@
@Override
public void copyFrom(PyBuffer src) throws IndexOutOfBoundsException, PyException {
- if (src instanceof BaseArrayBuffer) {
+ if (src instanceof BaseArrayBuffer && !this.overlaps((BaseArrayBuffer)src)) {
+ // We can do this efficiently, copying between arrays.
copyFromArrayBuffer((BaseArrayBuffer)src);
} else {
super.copyFrom(src);
}
}
+ private boolean overlaps(BaseArrayBuffer src) {
+ if (src.storage != this.storage) {
+ return false;
+ } else {
+ int low = calcLeastIndex(), high = calcGreatestIndex();
+ int srcLow = src.calcLeastIndex(), srcHigh = src.calcGreatestIndex();
+ return (srcHigh >= low && high >= srcLow);
+ }
+ }
+
private void copyFromArrayBuffer(BaseArrayBuffer src) throws IndexOutOfBoundsException,
PyException {
- checkWritable();
src.checkDimension(1);
int itemsize = getItemsize();
int count = getSize();
- // Block operation if different item or overall size (permit reshape)
+ // Block operation if different item or overall size
if (src.getItemsize() != itemsize || src.getSize() != count) {
throw differentStructure();
}
- for (int i = 0; i < count; i++) {
- int s = src.byteIndex(i), d = byteIndex(i);
- for (int j = 0; j < itemsize; j++) {
- storage[d++] = src.byteAtImpl(s++);
- }
- }
+ // We depend on the striding copyFrom() acting directly on the source storage
+ copyFrom(src.storage, src.index0, src.strides[0], 0, count);
}
- /**
- * Copy blocks of bytes, equally spaced in the source array, to locations equally spaced in the
- * destination array, which may be the same array. The byte at
- * <code>src[srcPos+k*srcStride+j]</code> will be copied to
- * <code>dst[dstPos+k*dstStride+j]</code> for <code>0≤k<count</code> and
- * <code>0≤j<size</code>. When the source and destination are the same array, the method
- * deals correctly with the risk that a byte gets written under the alias <code>dst[x]</code>
- * before it should have been copied referenced as <code>src[y]</code>.
- *
- * @param size of the blocks of bytes
- * @param src the source array
- * @param srcPos the position of the first block in the source
- * @param srcStride the interval between the start of each block in the source
- * @param dst the destination array
- * @param dstPos the position of the first block in the destination
- * @param dstStride the interval between the start of each block in the destination
- * @param count the number of blocks to copy
- */
- private static void slicedArrayCopy(int size, byte[] src, int srcPos, int srcStride,
- byte[] dst, int dstPos, int dstStride, int count) {}
-
@Override
protected ByteBuffer getNIOByteBufferImpl() {
// The buffer spans the whole storage, which may include data not in the view
diff --git a/src/org/python/core/buffer/BaseBuffer.java b/src/org/python/core/buffer/BaseBuffer.java
--- a/src/org/python/core/buffer/BaseBuffer.java
+++ b/src/org/python/core/buffer/BaseBuffer.java
@@ -536,18 +536,21 @@
int itemsize = getItemsize();
int count = getSize();
+ int byteLen = src.getLen();
// Block operation if different item or overall size (permit reshape)
- if (src.getItemsize() != itemsize || src.getLen() != count * itemsize) {
+ if (src.getItemsize() != itemsize || byteLen != count * itemsize) {
throw differentStructure();
}
- // XXX Re-think this using ByteBuffer when the API provides a byteIndex() method
- assert itemsize == 1;
- // XXX This only moves the first byte of each item
- for (int i = 0; i < count; i++) {
- storeAt(src.byteAt(i), i);
- }
+ /*
+ * It is not possible in general to know that this and src do not share storage. There is
+ * always a risk of incorrect results if we do not go via an intermediate byte array.
+ * Sub-classes may be able to avoid this.
+ */
+ byte[] t = new byte[byteLen];
+ src.copyTo(t, 0);
+ this.copyFrom(t, 0, 0, count);
}
@Override
diff --git a/src/org/python/core/buffer/BaseNIOBuffer.java b/src/org/python/core/buffer/BaseNIOBuffer.java
--- a/src/org/python/core/buffer/BaseNIOBuffer.java
+++ b/src/org/python/core/buffer/BaseNIOBuffer.java
@@ -221,61 +221,6 @@
}
}
- /**
- * {@inheritDoc}
- * <p>
- * The default implementation in <code>BaseNIOBuffer</code> deals with the general
- * one-dimensional case.
- */
- @Override
- public void copyFrom(PyBuffer src) throws IndexOutOfBoundsException, PyException {
-
- int length = getLen();
- int itemsize = getItemsize();
-
- // Valid operation only if writable and same length and itemsize
- checkWritable();
- if (src.getLen() != length || src.getItemsize() != itemsize) {
- throw differentStructure();
- }
-
- if (length > 0) {
- // Pick up attributes necessary to choose an efficient copy strategy
- int stride = getStrides()[0];
- int skip = stride - itemsize;
-
- ByteBuffer dst = getNIOByteBuffer();
-
- // Strategy depends on whether destination items are laid end-to-end or there are gaps
- if (skip == 0) {
- // Straight copy to contiguous bytes
- for (int i = 0; i < length; i++) {
- dst.put(src.byteAt(i));
- }
-
- } else if (itemsize == 1) {
- // Non-contiguous copy: single byte items
- int pos = dst.position();
- for (int i = 0; i < length; i++) {
- dst.put(pos, src.byteAt(i));
- pos += stride;
- }
-
- } else {
- // Non-contiguous copy: each time, and itemsize > 1
- int pos = dst.position();
- int s = 0;
- for (int i = 0; i < length; i++) {
- for (int j = 0; j < itemsize; j++) {
- dst.put(pos++, src.byteAt(s++));
- }
- pos += skip;
- }
- }
- }
-
- }
-
@Override
protected ByteBuffer getNIOByteBufferImpl() {
return storage.duplicate();
diff --git a/tests/java/org/python/core/PyBufferTest.java b/tests/java/org/python/core/PyBufferTest.java
--- a/tests/java/org/python/core/PyBufferTest.java
+++ b/tests/java/org/python/core/PyBufferTest.java
@@ -556,38 +556,37 @@
}
}
- /** Test method for {@link org.python.core.PyBuffer#copyFrom(byte[], int, int, int)}. */
+ /** Test method for {@link org.python.core.PyBuffer#copyFrom(PyBuffer)}. */
@Test
public void testCopyFromPyBuffer() {
- announce("copyFrom");
+ announce("copyFrom (PyBuffer)");
/*
* The test material (this time) presents a view that has n items of size i, that are spaced
* in the underlying buffer with stride s.
*/
- int n = spec.ref.length;
- int s = spec.getStride();
- int i = spec.getItemsize(); // = 1 in all test cases at time of writing
+ final int n = spec.ref.length;
+ final int p = spec.getStride();
// The material we copy to it should have these strides:
int[] srcStrides;
if (n < 2) {
- srcStrides = new int[] {i};
- } else if (s > 2 * i || s < -2 * i) {
- srcStrides = new int[] {i, s - i, s, s + i, -s + i, -s, -s - i};
- } else if (s == 2 * i || s == -2 * i) {
- srcStrides = new int[] {i, 2 * i, 3 * i, -i, -2 * i, -3 * i};
- } else { // ( s==i || s==-i )
- srcStrides = new int[] {i, 2 * i, -i, -2 * i};
+ srcStrides = new int[] {1};
+ } else if (p > 2 || p < -2) {
+ srcStrides = new int[] {1, p - 1, p, p + 1, -p + 1, -p, -p - 1};
+ } else if (p == 2 || p == -2) {
+ srcStrides = new int[] {1, 2, 3, -1, -2, -3};
+ } else { // ( s==1 || s==-1 )
+ srcStrides = new int[] {1, 2, -1, -2};
}
// Also need the maximum absolute value
int maxStride = 0;
- for (int t : srcStrides) {
- if (t > maxStride) {
- maxStride = t;
- } else if (-t > maxStride) {
- maxStride = -t;
+ for (int stride : srcStrides) {
+ if (stride > maxStride) {
+ maxStride = stride;
+ } else if (-stride > maxStride) {
+ maxStride = -stride;
}
}
@@ -597,7 +596,7 @@
// Make the source material to copy from, big enough to accommodate n strides
int srcMaterialSize = n * maxStride + maxOffset;
- ByteMaterial srcMaterial = new ByteMaterial(48, srcMaterialSize, i);
+ ByteMaterial srcMaterial = new ByteMaterial(48, srcMaterialSize, 1);
/*
* Now we need a series of PyBuffer views on the source data, sliced and offset according to
@@ -608,21 +607,18 @@
ExporterFactory[] factories = {spec.factory, new SimpleExporterFactory()};
for (ExporterFactory factory : factories) {
-
/*
* We'll use the same apparatus to create the source buffer as we use to make the test
* cases. The specifications for them will all be derived from this one:
*/
TestSpec original = new TestSpec(factory, srcMaterial);
-
/*
* Do this where the pattern of indices constituting src overlaps (or not) the pattern
* of view in challenging ways, including greater and smaller strides.
*/
-
for (int stride : srcStrides) {
for (int offset : srcOffsets) {
- int start = (stride > 0) ? offset : srcMaterialSize - offset - i;
+ int start = (stride > 0) ? offset : srcMaterialSize - offset - 1;
doTestCopyFrom(original, start, n, stride);
}
}
@@ -630,12 +626,7 @@
}
- // XXX Separately test where src is a view on is the same object.
-
-
-
- /** Helper function for {@link #testCopyFromPyBuffer()}
- */
+ /** Helper function for {@link #testCopyFromPyBuffer()} */
private void doTestCopyFrom(TestSpec original, int start, int n, int stride) {
// Derive sliced test material from the original
@@ -649,9 +640,11 @@
int p = spec.getStride();
String srcName = pair.obj.getClass().getSimpleName();
if (verbosity > 1) {
- System.out.printf(" copy src[%d:%d:%d] %s(%d) to obj[%d:%d:%d]\n",
- start, start+n*stride, stride, srcName,
- n, s, s + n * p, p);
+ int end = start + (n - 1) * stride + (stride > 0 ? 1 : -1);
+ int e = s + (n - 1) * p + (p > 0 ? 1 : -1);
+ System.out.printf(" copy from src[%d:%d:%d] %s(%d) to obj[%d:%d:%d]\n", //
+ start, end, stride, srcName, n, //
+ s, e, p);
}
// Initialise the destination object and view (have to do each time) from spec
@@ -684,6 +677,112 @@
}
}
+ /** Test method for {@link org.python.core.PyBuffer#copyFrom(PyBuffer)} when source is same. */
+ @Test
+ public void testCopyFromSelf() {
+ announce("copyFrom (self)");
+
+ // The test material (this time) presents a view of n bytes from a buffer of L bytes.
+ final int n = ref.length;
+ TestSpec original = spec.getOriginal();
+ if (spec.readonly || spec == original || n < 1) {
+ // We're only testing with sliced writable views
+ return;
+ }
+ final int p = spec.getStride();
+ final int L = original.ref.length;
+
+ /*
+ * We want to make another sliced view on the same test object, with the same number of
+ * items n, but different stride and/or offset. Strides above, equal to and below (if
+ * possible) the destination stride are of interest.
+ */
+ int[] srcStrides;
+ if (n < 2) {
+ srcStrides = new int[] {1};
+ } else if (p > 2 || p < -2) {
+ srcStrides = new int[] {1, p - 1, p, p + 1, -p + 1, -p, -p - 1};
+ } else if (p == 2 || p == -2) {
+ srcStrides = new int[] {1, 2, 3, -1, -2, -3};
+ } else { // ( p==1 || p==-1 )
+ srcStrides = new int[] {1, 2, -1, -2};
+ }
+
+ for (int srcStride : srcStrides) {
+ int absStride;
+ if (srcStride > 0) {
+ absStride = srcStride;
+ /*
+ * Compute the highest start index such that we can fit n items spaced at absStride
+ * into the buffer before reaching the end.
+ */
+ int maxOffset = L - 1 - absStride * (n - 1);
+ // There might not be such an start. If there is, we can do one or more tests.
+ if (maxOffset >= 0) {
+ // A positive-stepping slice could fit, for some start positions
+ int incOffset = 1 + maxOffset / 4;
+ for (int srcOffset = 0; srcOffset <= maxOffset; srcOffset += incOffset) {
+ doTestCopyFromSelf(srcOffset, srcStride, n);
+ }
+ }
+ } else {// srcStride < 0
+ absStride = -srcStride;
+ /*
+ * Compute the lowest start index such that we can fit n items spaced at absStride
+ * into the buffer before reaching the beginning.
+ */
+ int minOffset = absStride * (n - 1) + 1;
+ // There might not be such an start. If there is, we can do one or more tests.
+ if (minOffset < L) {
+ // A negative-stepping slice could fit, for some start positions
+ int incOffset = 1 + (L - 1 - minOffset) / 4;
+ for (int srcOffset = L - 1; srcOffset > minOffset; srcOffset -= incOffset) {
+ doTestCopyFromSelf(srcOffset, srcStride, n);
+ }
+ }
+ }
+ }
+ }
+
+ /** Helper function for {@link #testCopyFromPyBuffer()} */
+ private void doTestCopyFromSelf(int srcStart, int srcStride, int n) {
+
+ // Initialise the destination object and view (have to do each time) from spec
+ createObjAndView();
+
+ // Report the slice of the test object we are writing
+ int dstStart = spec.getStart();
+ int dstStride = spec.getStride();
+ String srcName = obj.getClass().getSimpleName();
+ if (verbosity > 1) {
+ int srcEnd = srcStart + (n - 1) * srcStride + (srcStride > 0 ? 1 : -1);
+ int dstEnd = dstStart + (n - 1) * dstStride + (dstStride > 0 ? 1 : -1);
+ System.out.printf(" copy from obj[%d:%d:%d] %s(%d) to obj[%d:%d:%d]\n", //
+ srcStart, srcEnd, srcStride, srcName, n, //
+ dstStart, dstEnd, dstStride);
+ }
+ assert !spec.readonly; // Test is only called if writable
+
+ // Our test is against the underlying object of which the view may be a slice
+ try (PyBuffer underlying = obj.getBuffer(PyBUF.FULL_RO)) {
+
+ // Take a snapshot before the call
+ byte[] before = bytesFromByteAt(underlying);
+
+ // Take the required slice-view to use as the source.
+ PyBuffer src = underlying.getBufferSlice(PyBUF.FULL_RO, srcStart, n, srcStride);
+ byte[] srcBytes = bytesFromByteAt(src);
+
+ // This is the call we are testing (a write operation).
+ view.copyFrom(src);
+
+ // Test that the corresponding bytes of the underlying object match data copied in
+ byte[] after = bytesFromByteAt(underlying);
+ ByteBufferTestSupport.checkWriteCorrect(before, after, srcBytes, 0, n, 1, dstStart,
+ dstStride);
+ }
+ }
+
/**
* Test method for {@link org.python.core.BufferProtocol#getBuffer()} and
* {@link org.python.core.PyBuffer#getBuffer()}.
--
Repository URL: https://hg.python.org/jython
More information about the Jython-checkins
mailing list