Get test_memoryview passing #625

gfmcknight · 2019-06-06T06:32:30Z

... Kind of.

I have a couple pieces of the implementation that I think need revision, namely:

When I added weakref support to memoryview, for some reason it wasn't releasing in time for the net4.5 target when run through make.ps1 tests. If I ran test_memoryview in any other circumstance the weakref seemed to release appropriately. I left it out for now, but it might be a good idea to investigate, given that we've seen something similar in weakref not released when expected #544.
The change to suboffsets (empty tuple instead of None) should probably be handled on the other side of the IBufferProtocol instead of in memoryview, right?

Store hashes of memoryview objects when computed once, add a check for buffer disposal in __enter__, update tolist() and tobytes() to handle offsets and strides, arrays, and throw errors when attempting comparison operators with a memoryview.

slozier

The weakref stuff is somewhat hard to deal with since it relies on the GC. Usually you can get it working by adding a gc.collect(). I'm typically fine with skipping these tests since the memory usually does get collected at some point.
Looking at the CPython source it seems like the tuple creation is done in the getter so this is probably fine. One thing we might consider is changing the IBufferProtocol.SubOffsets to the type PythonTuple?

Src/IronPython/Runtime/MemoryView.cs

slozier · 2019-06-06T11:33:34Z

Src/IronPython/Runtime/MemoryView.cs

@@ -390,6 +416,12 @@ public sealed class MemoryView : ICodeFormattable {
                    throw PythonOps.NotImplementedError("multi-dimensional sub-views are not implemented");
                }

+                // We expect to get a numeric value when setting a paricular
+                // element, which we will handle by breaking up into bytes
+                if (!(value is int)) {


May need to be careful here since the type could be a BigInteger or a user defined type (class test(int): pass)?

Ah I see; maybe I should be using something like Converter.TryConvertToInt32().

Or Int64 -- since the opcode can have 8 bytes. I need to add a test for this.

Src/IronPython/Runtime/Operations/StringOps.cs

Src/StdLib/Lib/test/test_memoryview.py

gfmcknight · 2019-06-06T15:50:37Z

If adding gc.collect() works to fix the weakref, would you also be okay with adding that into the test in case the implementation is ironpython or do you think it's better to leave the test disabled?

slozier · 2019-06-06T16:50:36Z

Yes, you can just add something like gc.collect() # required by IronPython, probably no need to conditionally exclude. I did something like that in test_deque (although I think it still fails once in a while).

suboffsets a PythonTuple.

gfmcknight · 2019-06-08T05:27:04Z

It still consistently failed when run from make.ps1. The test currrently calls support.gc_collect(). Maybe it has to do with Release vs Debug version?

Currently, I just have the test skipping for ironpython implementations.

slozier · 2019-06-09T01:55:54Z

It looks like changing the IsolationLevel seems to make test_weakref work. I'm fine with either skipping it (as long as an issue is filed -- even if it just says the test if failing for unknown reasons -- and you add a link to it in a comment in the source) or changing the IsolationLevel in AllCPythonCasesManifest.ini:

IsolationLevel=PROCESS # https://github.com/IronLanguages/ironpython3/issues/489

memoryviews. Set IsolationLevel=PROCESS for test_memoryview.

gfmcknight · 2019-06-17T06:29:06Z

Just a quick update since I left this hanging so long. Since test_file seems to be a case of a weakref not releasing as well, I think they should be in the same status so I think I'll mark set IsolationLevel=Process as above.

The remaining hangup that I'm working on is that my conversion to an int (line 421) doesn't work above for floats and the other types, and I'm trying to come up with a clean way of giving the correct responsibility of error handing to MemoryView and TypecodeOps.

slozier · 2019-06-17T13:39:54Z

Sounds like you might need a switch based on format to do the proper conversions?

memoryview.

gfmcknight · 2019-06-18T05:27:44Z

I guess I think it should be done in MemoryView. This is a little closer, and it includes the change to _itemsize.

Tomorrow I'll look into the set of typecodes that seem to be obsolete and other cleanup, and write some tests that are needed like the one for setting a double.

Src/IronPython/Runtime/TypecodeOps.cs

Src/IronPython/Runtime/MemoryView.cs

slozier · 2019-06-18T13:15:22Z

Src/IronPython/Runtime/MemoryView.cs

-            // TODO: BigInteger should also be merged into an integer once the int/long merge occurs
-            if (result is BigInteger || result is double || result is float || result is Bytes) {
+            // TODO: BigInteger/long/ulong should also be merged into an integer once the int/long merge occurs
+           if (result is BigInteger || result is double || result is float || result is Bytes) {


float probably needs to be cast to double.

I wonder if there should be a .NET primitive to Python type helper somewhere? Maybe there already is...

I believe GetPythonType works for primitive types. I'm not sure if that's the functionality you're referring to.

I meant something that essentially does what you do here: byte/short->int, float->double, long->BigInteger, and so on for all C# primitive types. GetPythonType does not do this since it will return you the Python version of the C# types (e.g. it will give you System.Int16 for shorts).

I wasn't able to find anything that does this, so I added one such method in PythonOps. I'm not entirely sure what the correct location for this would be.

Src/IronPython/Runtime/MemoryView.cs

Src/IronPython/Runtime/TypecodeOps.cs

gfmcknight · 2019-06-18T14:18:36Z

Seems like a serious case of me not cleaning up the things I started but didn't like. Sorry for making you review all of this!

slozier · 2019-06-18T16:41:08Z

I'm fine with reviewing, helps me get more familiar with the codebase. :)

Src/IronPython/Runtime/MemoryView.cs

doubles are not rounded in for memoryviews with the 'd' opcode. Clean up unused code.

gfmcknight · 2019-06-19T06:21:39Z

Src/IronPython/Runtime/MemoryView.cs

+        /// for this MemoryView.
+        /// </summary>
+        /// <param name="value">The value to be set.</param>
+        private void checkNumeric(BigInteger value) {


checkForOverflow. Should this be in TypecodeOps?

q and Q seem to be flipped here.

Edit: they were not.

Probably.

Might be lost in the noise, but it seems like we're doing a lot of converting back and forth: object -> BigInteger -> object -> primitive type -> byte[] -> primitive type -> object (and then SetItem does a bunch of the same work). Anyway, I guess we should get it working first before worrying about optimizations.

Yeah I think removing some of the conversions here is a good idea, for the moment this conversion to BigInteger makes the overflow check easier.

I was able to remove the conversion to BigInteger. Still thinking about the conversions to long and ulong, which are probably only necessary when we get a BigInteger value, to allow BitConverter to do what it wants.

in memoryview.

Python.

Src/IronPython/Runtime/TypecodeOps.cs

conversion to BigInteger for the overflow check.

slozier · 2019-06-27T13:23:30Z

Src/IronPython/Runtime/TypecodeOps.cs

@@ -188,7 +188,7 @@ class TypecodeOps {
                    return false; // All non-numeric types will not cause overflow.
            }

-            return value < minValue || value > maxValue;
+            return PythonOps.Compare(value, minValue) < 0 || PythonOps.Compare(value, maxValue) > 0;


Unfortunately calling Compare is probably even worse (performance wise) than converting to a BigInteger. Also, since Compare is equivalent to calling __cmp__ it will be removed from the codebase at some point.

I tried benchmarking setting, and indeed Compare is much slower. The current change is only a little faster (~10%) than converting to BigInteger (possibly within the margin of error -- it wasn't a very empirical process), but I think it's a little cleaner than expecting a BigInteger in TypecodeOps. Here's what I did to benchmark:

import time import array def benchmark_memoryview(mv): start_time = time.time() for j in range(2000): for i in range(4000): mv[i] = i + j end_time = time.time() return end_time - start_time mv = memoryview(array.array('i', [0]*4001)).cast('b')[1:16001].cast('i') print('int unaligned', benchmark_memoryview(mv)) mv = memoryview(array.array('i', [0]*4000)) print('int aligned', benchmark_memoryview(mv))

Also, the aligned benchmark was super slow because _shape was null, so I initialized it in the constructor.

slozier · 2019-06-27T13:29:36Z

Looking at the CI, it looks like the failures on Mono are weakref related? I would be fine with just skipping testing on Mono (by adding RunCondition=NOT $(IS_MONO) with a comment to AllCPythonCasesManifest.ini).

slozier

Looks fine to me overall. Did you want to get this merged in or was there more you wanted to do?

slozier · 2019-06-28T13:23:33Z

Src/IronPython/Runtime/MemoryView.cs

        }

        public PythonList tolist() {
            CheckBuffer();
-            return _buffer.ToList(_start, _end);


Is this a spot where we could have a fast path with _matchesBuffer && _step == 1?

Added, but only tested against the two test_memoryview tests.

gfmcknight · 2019-06-28T14:32:12Z

Other than the fast track, I don't think anything comes to mind. The last piece is indexing at a tuple, but I think that should be another pull request, and I don't believe it's blocking anything for IronPython.

Thanks for the patience and the helpful reviews with this one!

I believe that the remaining things to do after this is merged are:

Add a comment to weakref not released when expected #544
Add a comment to Some tests are failing when run without IsolationLevel=PROCESS #489

slozier · 2019-06-28T16:29:52Z

Thanks for the PR!

Get test_memoryview passing

01d5300

Store hashes of memoryview objects when computed once, add a check for buffer disposal in __enter__, update tolist() and tobytes() to handle offsets and strides, arrays, and throw errors when attempting comparison operators with a memoryview.

slozier reviewed Jun 6, 2019

View reviewed changes

Add IWeakReferenceable members to MemoryView. Make the type of

149cca9

suboffsets a PythonTuple.

Use _itemsize instead of (int)itemsize. Fix q opcode support for

89f8466

memoryviews. Set IsolationLevel=PROCESS for test_memoryview.

Check and coerce the types of objects before setting them in a

aa06b14

memoryview.

slozier reviewed Jun 18, 2019

View reviewed changes

gfmcknight commented Jun 19, 2019

View reviewed changes

Src/IronPython/Runtime/MemoryView.cs Outdated Show resolved Hide resolved

gfmcknight added 2 commits June 18, 2019 22:00

Make overflow checking occur in MemoryView. Add a test to ensure that

a0aa00e

doubles are not rounded in for memoryviews with the 'd' opcode. Clean up unused code.

Remove unsupported typecodes.

c5ac08d

gfmcknight commented Jun 19, 2019

View reviewed changes

gfmcknight added 3 commits June 19, 2019 08:03

Add some tests for memoryview overflows.

4c4349f

Move the overflow check to TypecodeOps and add more tests for overflows

e83e8b6

in memoryview.

Add a helper to convert to a nearby type which is also a primitive in

a11cde5

Python.

gfmcknight commented Jun 22, 2019

View reviewed changes

Src/IronPython/Runtime/TypecodeOps.cs Outdated Show resolved Hide resolved

Disallow the setting of doubles to discrete-valued opcodes. Remove the

086d5c2

conversion to BigInteger for the overflow check.

slozier reviewed Jun 27, 2019

View reviewed changes

gfmcknight added 2 commits June 27, 2019 22:05

Remove uses of PythonOps.compare(). Disable test_memoryview on Mono.

ccc9986

Always initialize the _shape of MemoryViews.

100cb23

gfmcknight force-pushed the master branch from 98512d9 to 100cb23 Compare June 28, 2019 05:45

slozier approved these changes Jun 28, 2019

View reviewed changes

Add a fast track on tolist().

6018ed5

slozier merged commit 434c5ca into IronLanguages:master Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get test_memoryview passing #625

Get test_memoryview passing #625

gfmcknight commented Jun 6, 2019

slozier left a comment

slozier Jun 6, 2019

gfmcknight Jun 6, 2019

gfmcknight Jun 8, 2019

gfmcknight commented Jun 6, 2019

slozier commented Jun 6, 2019

gfmcknight commented Jun 8, 2019 •

edited

slozier commented Jun 9, 2019

gfmcknight commented Jun 17, 2019

slozier commented Jun 17, 2019

gfmcknight commented Jun 18, 2019 •

edited

slozier Jun 18, 2019

gfmcknight Jun 19, 2019

slozier Jun 19, 2019

gfmcknight Jun 22, 2019

gfmcknight commented Jun 18, 2019

slozier commented Jun 18, 2019

gfmcknight Jun 19, 2019

gfmcknight Jun 19, 2019 •

edited

slozier Jun 19, 2019

gfmcknight Jun 19, 2019

gfmcknight Jun 27, 2019

slozier Jun 27, 2019

gfmcknight Jun 28, 2019

slozier commented Jun 27, 2019

slozier left a comment

slozier Jun 28, 2019

gfmcknight Jun 28, 2019

gfmcknight commented Jun 28, 2019

slozier commented Jun 28, 2019

Get test_memoryview passing #625

Get test_memoryview passing #625

Conversation

gfmcknight commented Jun 6, 2019

slozier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfmcknight commented Jun 6, 2019

slozier commented Jun 6, 2019

gfmcknight commented Jun 8, 2019 • edited

slozier commented Jun 9, 2019

gfmcknight commented Jun 17, 2019

slozier commented Jun 17, 2019

gfmcknight commented Jun 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfmcknight commented Jun 18, 2019

slozier commented Jun 18, 2019

Choose a reason for hiding this comment

gfmcknight Jun 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slozier commented Jun 27, 2019

slozier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfmcknight commented Jun 28, 2019

slozier commented Jun 28, 2019

gfmcknight commented Jun 8, 2019 •

edited

gfmcknight commented Jun 18, 2019 •

edited

gfmcknight Jun 19, 2019 •

edited