Opened 5 years ago

Last modified 5 years ago

#22793 new enhancement

New LazyImport implementation based on lazy_object_proxy

Reported by: embray Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers:
Report Upstream: N/A Work issues:
Branch: u/embray/lazy-import-proxy (Commits, GitHub, GitLab) Commit: e58d61d89903419e773643c495d6f50931929630
Dependencies: #22792 Stopgaps:

Status badges

Description (last modified by embray)

This adds `lazy_object_proxy` as a dependency (see #22792), and rewrites the sage.misc.lazy_import.LazyImport class on top of it. This doesn't change anything else about how the lazy_import mechanism is implemented and is mostly independent of any such changes (e.g. #22752).

Pros

  • In the spirit of tickets like #21805, removes more code than it adds, replacing Sage-specific code with robust solutions from the Python community...
  • ...meaning less code for us to maintain and keep forward-compatible with future Python versions (in principle, but see also the first "con").
  • This solution is probably faster, being written purely in C and with no Cython-related overhead. The old implementation in Sage required a function call for every indirection to the proxied object (albeit a cpdef function). The lazy_object_proxy implementation in principle also has a plain C function call (Proxy__ensure_wrapped), but with typical compiler optimizations this is actually eliminated entirely for the common case where the proxied object has been initialized. I haven't done any benchmarks though, but could if requested.
  • Maintainer is an active member of the Python community and not likely to abandon the project or disappear (hard to quantify of course, but he's someone whose work I'm familiar with).

Cons

  • Effectively same amount of code to maintain if we ever need upstream changes for Sage--upstream can be more difficult to deal with than fixing code in Sage.

That said, the implementation of lazy_object_proxy is very generic and not likely to require deep changes for Sage. The old LazyImport has never required deep structural changes either.

Change History (16)

comment:1 Changed 5 years ago by jdemeyer

Won't this conflict with #22755?

comment:2 Changed 5 years ago by embray

Probably; I didn't see that ticket.

comment:3 Changed 5 years ago by embray

  • Description modified (diff)

Probably; I didn't see that ticket.

comment:4 Changed 5 years ago by embray

I mean, I would say this mostly supersedes #22755, though there are some changes from that ticket that would still be applicable, and I could integrate.

comment:5 Changed 5 years ago by jdemeyer

Do you have further plans regarding lazy imports?

I would very much like to have an implementation of lazy imports which works in Cython. At this point, it's not clear to me whether this ticket makes that easier or harder. So I'd prefer to leave this ticket pending until somebody provides a Cython implementation of lazy imports based on this Proxy object.

comment:6 follow-up: Changed 5 years ago by embray

I do, but they're not 100% dependent on this; it was just an idea. What do you mean, in this case, by "works with Cython"?

comment:7 in reply to: ↑ 6 Changed 5 years ago by jdemeyer

Replying to embray:

I do, but they're not 100% dependent on this; it was just an idea.

Right, I do agree that the implementation of the lazy import mechanism (lazy_import) and the lazy object (class LazyImport) is mostly orthogonal. But it's also not 100% orthogonal, and the details might matter.

What do you mean, in this case, by "works with Cython"?

As I wrote on #22752, an implementation of the lazy import mechanism which works equally well for Python and Cython.

comment:8 in reply to: ↑ description Changed 5 years ago by jdemeyer

Replying to embray:

  • This solution is probably faster

Arithmetic is actually about a factor 3 slower...

With #22755:

sage: from sage.misc.lazy_import import LazyImport; sage.all.one = 1; a = LazyImport('sage.all', 'one'); timeit('a+a', number=1000000, repeat=100)
1000000 loops, best of 100: 81.1 ns per loop

With #22793:

sage: from sage.misc.lazy_import import LazyImport; sage.all.one = 1; a = LazyImport('sage.all', 'one'); timeit('a+a', number=1000000, repeat=100)
1000000 loops, best of 100: 220 ns per loop

To be honest, this surprised me too. Maybe it's because of the Python subclass LazyImport on top of Proxy?

For completeness, with vanilla Sage 8.0.beta0:

sage: from sage.misc.lazy_import import LazyImport; sage.all.one = 1; a = LazyImport('sage.all', 'one'); timeit('a+a', number=1000000, repeat=100)
1000000 loops, best of 100: 175 ns per loop
Last edited 5 years ago by jdemeyer (previous) (diff)

comment:9 follow-up: Changed 5 years ago by nbruin

Possible cause for the slowdown: the overhead of the factory function, which is a full-blown python function call, rather than the cdef _get_object we had originally. It looks like the the right amount of overhead (python function calls are expensive, and we're getting two of them in the benchmark).

Of course, the whole idea of LazyImport? is that the proxy shim removes itself, which is carefully avoided in the test above. So we are benchmarking something that should be avoided ...

comment:10 in reply to: ↑ 9 Changed 5 years ago by jdemeyer

Replying to nbruin:

Possible cause for the slowdown: the overhead of the factory function, which is a full-blown python function call

If I understand things correctly, the factory function should only be called once for a given object, so that shouldn't matter.

comment:11 Changed 5 years ago by jdemeyer

The overhead from the Python subclass is significant:

sage: from lazy_object_proxy import Proxy
sage: class PyProxy(Proxy): pass
sage: def factory(): return 1
sage: a = Proxy(factory); timeit('a+a', number=1000000, repeat=100)
1000000 loops, best of 100: 82.7 ns per loop
sage: a = PyProxy(factory); timeit('a+a', number=1000000, repeat=100)
1000000 loops, best of 100: 222 ns per loop

So, the Proxy is equally fast as LazyImport, but the additional Python subclass makes it a lot slower.

comment:12 Changed 5 years ago by embray

Interesting, thanks for the testing. I honestly have no idea why being a subclass would be slower since normally a subtype will inherit most of the type slots, especially for numeric operations, so there should be no subclass traversal overhead. I'll look into it though.

I haven't looked into how this Proxy implementation fits in with Cython. But one thing I like very much about it is the cleanness and simplicity of the implementation. Sure, the version currently in Sage is also simple as Cython code, but I find it more difficult to reason about how Cython is actually doing things than a plain C implementation, especially in cases like this. If nothing else it might be nice to fork this code and adapt it to work the way we need it to. Or at the very least modify the LazyProxy implementation to be a little more generic, since there's not much about it that has specifically to do with imports, except for the details in _get_object.

All that said, this isn't the most important aspect of reworking the lazy import system--it seemed like an opportunity to simplify things by factoring out one of its moving parts a little more.

comment:13 follow-up: Changed 5 years ago by embray

I see where the slowdown is happening: In the Proxy_Add function (and other numerical methods) it does a PyObject_IsInstance check on the operands to see if they are proxy objects (just as in the existing implementation in Sage). For the base class this isn't a problem because it's just a pointer comparison. For a subclass, however, it has to go through the full recursive isinstance check.

comment:14 in reply to: ↑ 13 Changed 5 years ago by jdemeyer

Replying to embray:

I see where the slowdown is happening: In the Proxy_Add function (and other numerical methods) it does a PyObject_IsInstance check on the operands to see if they are proxy objects (just as in the existing implementation in Sage). For the base class this isn't a problem because it's just a pointer comparison. For a subclass, however, it has to go through the full recursive isinstance check.

Why is the overhead so much for a MRO with 3 elements? That would be just 3 pointer comparisons instead of 1. That cannot explain the whole slowdown.

comment:15 Changed 5 years ago by embray

Unfortunately, for inexact matches PyObject_IsInstance is a lot more complicated than that due to the need to support __instancecheck__. That would definitely account for the amount of slowdown.

comment:16 Changed 5 years ago by embray

This small patch speeds it up quite a bit:

  • src/lazy_object_proxy/cext.c

    diff --git a/src/lazy_object_proxy/cext.c b/src/lazy_object_proxy/cext.c
    index 684bcf7..f6183a2 100644
    a b  
    99#endif
    1010
    1111#define Proxy__WRAPPED_REPLACE_OR_RETURN_NULL(object) \
    12     if (PyObject_IsInstance(object, (PyObject *)&Proxy_Type)) { \
     12    if (PyObject_TypeCheck(object, &Proxy_Type) || \
     13        PyObject_IsInstance(object, (PyObject *)&Proxy_Type)) { \
    1314        object = Proxy__ensure_wrapped((ProxyObject *)object); \
    1415        if (!object) return NULL; \
    1516    }
In [1]: from lazy_object_proxy import Proxy

In [2]: a = Proxy(int)

In [3]: str(a)
Out[3]: '0'

In [4]: %timeit -n 1000000 -r 100 a + a
1000000 loops, best of 100: 57.4 ns per loop

In [5]: class MyProxy(Proxy): pass

In [6]: b = MyProxy(int)

In [7]: str(b)
Out[7]: '0'

In [8]: %timeit -n 1000000 -r 100 b + b
1000000 loops, best of 100: 62.1 ns per loop

PyObject_TypeCheck works more like you seem to be expecting. I could probably speed it up even a tad more for direct inheritance with a direct check of tp_base but it's probably not worth it.

Note: See TracTickets for help on using tickets.