MAINT: Speedup field access by removing unneeded safety checks #6208

ahaldane · 2015-08-14T18:11:29Z

#5548 causes a big slowdown in the specific case of accessing/assigning fields of structured arrays with large (eg 1kb) dtypes. Oops! I noticed this while investigating #1984. The problem is #5548 implemented an algorithm which checks view safety byte-by-byte (slow) under the assumption that most dtypes are only a small number of bytes. That's bad if the dtype is very large, as in the test script of #1984.

One fix is to speed the safety-check algorithm up, but it also turns out that the checks aren't needed in many cases. For example, during field indexing they aren't needed because we're merely viewing fields that already exist, which is therefore safe. So I've bypassed the safety checks by rewriting that indexing code (in C) so it avoids using PyArray_View.

I also made corresponding voidtype methods call the ndarray methods to get the same benefits. Incidentally this also enables some functionality that was missing from voidtype relative to ndarray. Eg, you can now do arr[0][['a', 'b']] and it will give you a more correct error message.

Safety checks are still important for get/setfield and for views. But even there we can skip the checks early if we know that the datatypes aren't structured or aren't of object type.

Timing info for the test script in #1984, run as ./test.py 10000

Numpy 1.9.2:

array,  B['a'][:] = A['a']      0.00852 seconds
array,  B['a']    = A['a']      0.00648 seconds
scalar, B['a'][:] = A['a']      0.016 seconds
scalar, B['a']    = A['a']      0.132 seconds

Master:

array,  B['a'][:] = A['a']      16.4 seconds
array,  B['a']    = A['a']      16.3 seconds
scalar, B['a'][:] = A['a']      16.1 seconds
scalar, B['a']    = A['a']      16.6 seconds

This PR:

array,  B['a'][:] = A['a']      0.00733 seconds
array,  B['a']    = A['a']      0.00673 seconds
scalar, B['a'][:] = A['a']      0.0171 seconds
scalar, B['a']    = A['a']      0.0151 seconds

Later I'll look into a faster algorithm for the structured safety checks.

I think this PR also fixes #1984, if 0.0151s is fast enough!

By the way, I think get/setfield are no longer used anywhere internally in numpy.

seberg · 2015-08-19T08:45:13Z

Do we have to consider this a 1.10 regression?

ahaldane · 2015-08-19T15:42:22Z

I don't yet know enough about the dev process to answer that, but note it's probably a pretty rare problem:

The slowdown should only be noticeable if you are taking 1000s of views of an array with a huge dtype. I expect that the number of people using huge dtypes (eg, large subarrays) is pretty small, and then the number of people also taking 1000s of views with such a dtype is even smaller.

seberg · 2015-08-19T16:29:51Z

OK, good, then I assume it does not have priority, just wondered.

ahaldane · 2015-10-13T16:43:11Z

I'll write up a short description of the code changes in a little bit, to help any reviewers.

seberg · 2015-10-13T16:56:40Z

Not sure you have to worry about it too much, back in the day I wanted to know if we need to make sure it makes it into 1.10. I thought it did not sound like it would be necessary (which apparently was not quite right). After that, never looked at it. If you think it makes reviewing much easier, go ahead I guess.

pv · 2015-10-13T17:34:04Z

Maybe add a benchmark for it? pv@1dff685

ahaldane · 2015-10-13T18:05:58Z

Great, I tried adding your commit. Hopefully that is the right way to do it.

charris · 2015-10-13T21:47:39Z

numpy/core/_internal.py

-    """ Given a structured array and a sequence of field names
-    construct new array with just those fields.
+def _copy_fields(ary):
+    """Returns a copy of a structured array with padding/unknown bytes between


Please keep the summary lines < 80 characters. Maybe

"""Return copy of structured array with unlabled bytes between fields removed. """

Or some such.

Or maybe even

"""Return copy of structured array with padding between fields removed. """

charris · 2015-10-13T22:05:53Z

Do we have to consider this a 1.10 regression?

Looks like it ;)

This is a pretty extensive refactoring, so it makes me a bit nervous. Someone needs to do a deep review of the code, which I haven't managed yet. Did you ever profile the original to see just where the bottlenecks were?

ahaldane · 2015-10-14T15:33:44Z

I don't think profiling is needed: I know the view safety checks are slow for large datatypes, and I know they are used when indexing fields. So the solution is to (safely) skip the checks here.

ahaldane · 2015-10-18T17:26:16Z

I am also not against reverting #5548. I knew that the algorithm there was slow, but didn't expect it to be such a problem. Given the extent of the changes needed here, I'm thinking #5548 needs to be given more thought - eg maybe the alternate approach I discussed but decided against would avoid this slowdown.

I just tried reverting it on my local setup, there does not seem to be any problem in doing so. (I just needed one extra small change to account for extra safety features I introduced elsewhere.) Reverting is the easy fix to these speed issues, and I could reintroduce a better version of #5548 for 1.11 or later. Any opinions?

charris · 2015-10-18T17:37:11Z

@ahaldane If you fix the nit I'll put this in so it can get some testing.

charris · 2015-10-18T17:41:43Z

Reversion would be the easy fix. I think it comes down to whether of not you would like to take time to rethink the issue and put up a cleansheet solution or continue to pursue the current fixes.

Bypass unneeded "view" safety-checks in `array_subscript` and `array_assign_subscript`, by avoiding use of `PyArray_View`.

Bypass unneeded "view" safety checks in voidtype_ subscript/assignment methods, by falling back to ndarray methods which skip the checks.

Skip safety-checks in views as long as neither old or new dtypes of view may have objects.

ahaldane · 2015-10-18T19:45:40Z

Looking over it all again (and adding a minor tweak), I think merging this is better than reverting #5548. #5548 is probably the right solution, at least until a new dtype system comes out. If not, it's pretty easy to back out of later, and it only introduces a performance problem.

If you do merge, I've left #6467 open so we can wait for people to report back that their particular case is fixed.

charris · 2015-10-18T20:14:50Z

OK, let's give it a shot. Thanks Allan.

MAINT: Speedup field access by removing unneeded safety checks

ahaldane force-pushed the fast_field_subscript branch 21 times, most recently from 4573b88 to 290213c Compare August 15, 2015 01:08

charris added component: numpy._core 03 - Maintenance labels Sep 21, 2015

ahaldane mentioned this pull request Oct 13, 2015

performance regression for record array access in numpy 1.10.1 #6467

Closed

seberg added the 06 - Regression label Oct 13, 2015

ahaldane force-pushed the fast_field_subscript branch from 290213c to 1dff685 Compare October 13, 2015 18:04

charris added this to the 1.10.2 release milestone Oct 13, 2015

charris reviewed Oct 13, 2015
View reviewed changes

dkirkby mentioned this pull request Oct 18, 2015

Slow creation of spAll full db dkirkby/bossdata#106

Closed

ahaldane force-pushed the fast_field_subscript branch from 1dff685 to 16a2ab1 Compare October 18, 2015 17:42

ahaldane and others added 4 commits October 18, 2015 15:15

MAINT: Speedup field access by removing unneeded safety checks (1/3)

b3ce7a6

Bypass unneeded "view" safety-checks in `array_subscript` and `array_assign_subscript`, by avoiding use of `PyArray_View`.

MAINT: Speedup field access by removing unneeded safety checks (2/3)

9d1a7c9

Bypass unneeded "view" safety checks in voidtype_ subscript/assignment methods, by falling back to ndarray methods which skip the checks.

MAINT: Speedup field access by removing unneeded safety checks (3/3)

37382ac

Skip safety-checks in views as long as neither old or new dtypes of view may have objects.

PERF: add 0d structured indexing benchmark

8cf5b50

ahaldane force-pushed the fast_field_subscript branch from 16a2ab1 to 8cf5b50 Compare October 18, 2015 19:21

charris added a commit that referenced this pull request Oct 18, 2015

Merge pull request #6208 from ahaldane/fast_field_subscript

c3b48b9

MAINT: Speedup field access by removing unneeded safety checks

charris merged commit c3b48b9 into numpy:master Oct 18, 2015

This was referenced Oct 18, 2015

Fix issues with zero-width string fields #6430

Merged

EHN: ASC/DESC ordering in sort argsort, and argpartition. #6222

Closed

WIP: make views of different type checks less strict #5508

Closed

charris mentioned this pull request Oct 18, 2015

Backport 6208: MAINT: Speedup field access by removing unneeded safety checks #6502

Merged

embray mentioned this pull request Oct 22, 2015

FITS: Performance issue astropy/astropy#4259

Closed

ahaldane mentioned this pull request Oct 26, 2015

Disable view safety checks #6562

Merged

charris removed this from the 1.10.2 release milestone Oct 27, 2015

ahaldane mentioned this pull request Feb 16, 2016

numpy.void and numpy.ma.mvoid with structured dtypes should support indexing with multiple fields #7262

Closed

ahaldane mentioned this pull request Jan 23, 2017

ENH: Make it possible to call .view on object arrays #8514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Speedup field access by removing unneeded safety checks #6208

MAINT: Speedup field access by removing unneeded safety checks #6208

ahaldane commented Aug 14, 2015

seberg commented Aug 19, 2015

ahaldane commented Aug 19, 2015

seberg commented Aug 19, 2015

ahaldane commented Oct 13, 2015

seberg commented Oct 13, 2015

pv commented Oct 13, 2015

ahaldane commented Oct 13, 2015

charris Oct 13, 2015

charris Oct 13, 2015

charris commented Oct 13, 2015

ahaldane commented Oct 14, 2015

ahaldane commented Oct 18, 2015

charris commented Oct 18, 2015

charris commented Oct 18, 2015

ahaldane commented Oct 18, 2015

charris commented Oct 18, 2015

MAINT: Speedup field access by removing unneeded safety checks #6208

MAINT: Speedup field access by removing unneeded safety checks #6208

Conversation

ahaldane commented Aug 14, 2015

seberg commented Aug 19, 2015

ahaldane commented Aug 19, 2015

seberg commented Aug 19, 2015

ahaldane commented Oct 13, 2015

seberg commented Oct 13, 2015

pv commented Oct 13, 2015

ahaldane commented Oct 13, 2015

charris Oct 13, 2015

Choose a reason for hiding this comment

charris Oct 13, 2015

Choose a reason for hiding this comment

charris commented Oct 13, 2015

ahaldane commented Oct 14, 2015

ahaldane commented Oct 18, 2015

charris commented Oct 18, 2015

charris commented Oct 18, 2015

ahaldane commented Oct 18, 2015

charris commented Oct 18, 2015