@mhvk - this looks great from the description, but clearly needs a decent chunk of time to dig into and evaluate. So it's on my radar, but probably not in the next few days...

No worries - but if you do have a small amount of time, it would be lovely to get #8854 out of the way...

@mvhk - have you had a look at https://www.python.org/dev/peps/pep-0487/ ? This is available as of Python 3.6. Not clear if the __slots__ stuff is possible, but it would be nice to avoid metaclasses if possible.

(Taking this out of the inline comments as I think it goes to the heart of the matter.)

@taldcroft - The approach with just defining descriptors on the class is indeed a lot clearer!

The main reason I went for the meta-class approach was to keep the API similar, with its attr_names and attrs_from_parent - which is why there are relatively few changes to Info subclasses.

The question then is whether we are happy to change it, which in turn depends on whether anybody outside of astropy relies on it. In principle, I guess the metaclass could help here, by doing the auto-generation while emitting a deprecation warning. But of course easier if we can do without it...

p.s. Have been meaning to look at the effects of PEP 487... Definitely useful for simple registration like Time fornats.

.. was to keep the API similar, with its attr_names and attrs_from_parent

Ah yes, I didn't immediately think of the API. That's possibly thorny... So what about some radical thoughts just for the sake of conversation:

• What if we left the existing DataInfo hierarchy in place for external usage (but deprecate) and then made a brand new infrastructure that keeps most of the public API but is not constrained to implement in the same way. For the sake of discussion call it XDataInfo or XMixinInfo.
• attr_names and attrs_from_parents can be done (for reading) with __set_name__ in the descriptors.
• Basically saying, Info is a honking good idea, let's make it better and faster with a reboot. In practice it probably won't be radically different, but just a chance to take stock of what is there and make it nicer.
• I think that in the rest of astropy there are not too many places we would need to change to reflect this additional XDataInfo class hierarchy (but I haven't tried looking).
• Our Astropy classes would have e.g. class QuantityInfoBase(XParentDtypeInfo), class TimeInfo(XMixinInfo), etc.

Having given a decent first pass through this code and seen the timing, it looks great! So it might be a reasonable strategy to get this in shape to merge and separately think about a bigger redesign to make the code more elegant / readable and maybe even faster.

OK, I've implemented the suggested changes. I like the idea of merging this, and worrying about a simpler design separately. The one outstanding question, I think, is whether it is worth the hassle with __slots__ to prevent people from sticking arbitrary attributes on info.

Here are some timing results on my laptop with your branch and master. I get qualitatively similar results. This also explores the speedup of a different strategy (attrs are stored as object attributes, not dict elements).

from weakref import ref as weakref
from astropy.utils.data_info import DataInfo
from astropy.table import MaskedColumn

%astro

astropy=4.0.dev25224

DataInfo._attr_defaults  # Shows this is the faster-info-via-meta-class branch

{'dtype': dtype('O')}

class A:
    info = DataInfo()

a = A()

a.info.description = 'asdf'

timeit a.info

463 ns ± 3.27 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

MASTER: 2.5 µs ± 43.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

timeit a.info.description

845 ns ± 13.7 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

MASTER: 3.39 µs ± 64.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

a.__dict__

{'info': dtype = object
 shape = --
 description = asdf
 class = A
 n_bad = 0}

class SimpleDataInfo:
    """Simplest possible data info container"""
    __slots__ = ['name', 'description', 'dtype', 'meta', 'unit', '_parent_ref']

class B:
    """Use SimpleDataInfo via a class property"""
    @property
    def info(self):
        try:
            info = self._info
        except AttributeError:
            info = self._info = SimpleDataInfo()
        finally:
            # info._parent_ref = self # weakref(self)
            return info            

b = B()

b.info.name = 'asdf'

timeit b.info.name

207 ns ± 6.02 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

class FDataInfo:
    """Descriptor that roughly matches the property above"""
    def __get__(self, instance, owner_cls):
        try:
            info = instance._info
        except AttributeError:
            if instance is None:
                # This is an unbound descriptor on the class
                self._parent_cls = owner_cls
                return self
            else:
                info = instance._info = SimpleDataInfo()
        finally:
            # info._parent_ref = weakref.ref(instance)
            return info

class C:
    info = FDataInfo()

c = C()

c.info.description = 'hello'

timeit c.info.description

231 ns ± 4.75 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

m = MaskedColumn([1, 2])
m.info.description = 'asdf'

timeit m.description

50 ns ± 0.801 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

timeit m.info.description

1.01 µs ± 10.2 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

junk = SimpleDataInfo()
m.junk = junk

timeit m.junk

58.1 ns ± 0.271 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

m.junk.name = 'name'

timeit m.junk.name

86.1 ns ± 0.644 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

@mvhk - planning to get back to this now that you are returned from vacation. My recollection is that there were some things to explore, but that this PR itself was in reasonably good shape and definitely helps performance.

Yes, I think the only real question here is whether we want to explicitly prevent people from setting arbitrary attributes on info (as is the case currently, as which requires __slots__) or whether we actually do not care (in which case we can get rid of __slots__).

👍 on explicitly preventing people from setting arbitrary attributes on info. That matches the current release behavior and I think it prevents mistakes far more often than being a limitation. Along those lines, I have often done things like q.name = name on a Quantity when I meant q.info.name.

The current implementation as slots is thus fine to me. If we even move to a different implementation that does the same thing then that will also be fine.

OK, I reinstated the test and fixed the code so that the appropriate message is given again. The difference was that now there always is a weakref - it can just be dead, in which case _parent_ref() returns None.