Remove platform.architecture() in FITS _array_to_file to improve speed on Mac - #2345

To profile I put the test_lossless_gzip_compression test in a standalone function (https://gist.github.com/cdeil/11154616) and ran this profile which shows that 12 out of 13 seconds are spent in platform.architecture():

In [1]: %run slow.py

In [2]: %prun test()
WARNING: Overwriting existing file 'test.fits'. [astropy.io.fits.file]

         483810 function calls (483424 primitive calls) in 13.113 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
     2002    4.721    0.002    4.721    0.002 {posix.read}
     2010    3.275    0.002    3.275    0.002 {method 'read' of 'file' objects}
     2002    1.395    0.001    1.395    0.001 {posix.fork}
     2002    0.616    0.000    7.303    0.004 subprocess.py:1186(_execute_child)
     2002    0.422    0.000    0.448    0.000 subprocess.py:1207(_close_in_parent)
     2002    0.312    0.000    7.780    0.004 subprocess.py:648(__init__)
        2    0.311    0.156    0.313    0.157 {astropy.io.fits.compression.compress_hdu}
     4004    0.240    0.000    0.299    0.000 subprocess.py:1370(wait)
     2002    0.170    0.000   12.020    0.006 platform.py:1077(architecture)
     2002    0.163    0.000   11.776    0.006 platform.py:1018(_syscmd_file)

Seems fine to cache the architecture -- by definition should be very unlikely to change during the runtime of the application ;)

I just saw that there's a note at https://docs.python.org/2/library/platform.html#platform.architecture that suggests that is_64bits = sys.maxsize > 2**32 might be more reliable than platform.architecture()[0] == '64bit'. Should we do that?

I'll wait for @embray to comment why he put the extra platform.architecture()[0] == '64bit' check into the current version:

    # Implements a workaround for a bug deep in OSX's stdlib file writing
    # functions; on 64-bit OSX it is not possible to correctly write a number
    # of bytes greater than 2 ** 32 and divisble by 4096 (or possibly 8192--
    # whatever the default blocksize for the filesystem is).
    # This issue should have a workaround in Numpy too, but hasn't been
    # implemented there yet: https://github.com/astropy/astropy/issues/839
    osx_write_limit = (2 ** 32) - 1

    if (sys.platform == 'darwin' and platform.architecture()[0] == '64bit' and
            arr.nbytes >= osx_write_limit + 1 and arr.nbytes % 4096 == 0):

No, relying on sys.maxsize here isn't actually appropriate since this fix is to work around a bug in OSX's own libc. Though I guess we could remove the check entirely since most OSX systems that matter are going to be 64-bit anyway.

I think the deeper question is why does platform.architecture() take 12 seconds? That seems silly. Caching its result also needs to be handled with care since in principle a task could be farmed out to multiple machines on different platforms. So the only way to do this safely is once per process. I'd say the platform.architecture() could just be skipped here.

Oh, when you mentioned caching though I didn't realize this call was happening in a loop. I'm sort of surprised it gets called so many times because usually that function is just used to write one array in full. But if used with variable-length arrays (as would be the case with a compressed image) then maybe it gets called that many times. So caching the check in that context makes sense.

All the more reason to just go ahead and remove the platform.architecture() call though. The difference isn't going to matter all that much for the now rare case of running on a 32-bit Mac.

Okay, the attached fix should make this much better. Thanks for calling this out!

Changed the title to reflect the proposed fix.

Just to confirm ... the attached fix by @embray does reduce the runtime for this test from 13 seconds to 1 second, to 👍 to merge.

Haha, wow. Sorry about that :D

Looks good - merging!