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: |
Description (last modified by )
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). Thelazy_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
comment:2 Changed 5 years ago by
Probably; I didn't see that ticket.
comment:4 Changed 5 years ago by
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
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: ↓ 7 Changed 5 years ago by
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
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
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
comment:9 follow-up: ↓ 10 Changed 5 years ago by
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
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
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
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: ↓ 14 Changed 5 years ago by
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
Replying to embray:
I see where the slowdown is happening: In the
Proxy_Add
function (and other numerical methods) it does aPyObject_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
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
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 9 9 #endif 10 10 11 11 #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)) { \ 13 14 object = Proxy__ensure_wrapped((ProxyObject *)object); \ 14 15 if (!object) return NULL; \ 15 16 }
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.
Won't this conflict with #22755?