Opened 10 years ago

Last modified 8 years ago

#13933 closed defect

BuiltinFunction.__call__ is unnecessarily slow — at Version 7

Reported by: Robert Bradshaw Owned by: Burcin Erocal
Priority: major Milestone: sage-5.11
Component: symbolics Keywords: sd48
Cc: Burcin Erocal Merged in:
Authors: Robert Bradshaw Reviewers: Burcin Erocal
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Robert Bradshaw)

This was discovered when looking at #12615. Basically, foo(x) eventually calls x.foo() if it exists, but only after doing a lot of symbolic work (resulting in an order of magnitude slowdown for that example).

Apply only:

  • 13933-slow-call.v2.patch
  • 13933-doctests.patch
  • 13933-more-doctests.patch

Change History (13)

Changed 10 years ago by Robert Bradshaw

Attachment: 13933-slow-call.patch added

Any reason why this isn't a good idea?

comment:1 Changed 10 years ago by Robert Bradshaw

Cc: Burcin Erocal added
Status: newneeds_review

comment:2 Changed 10 years ago by Burcin Erocal

Authors: Robert Bradshaw
Reviewers: Burcin Erocal
Status: needs_reviewneeds_info

In principle it is a good idea. I did something similar in attachment:trac_1173-move_call.patch:ticket:1173.

Putting this in the Function class is overkill. User defined symbolic functions also inherit from that class. People might be very surprised if all symbolic functions magically tried to call methods of the same name before doing anything else. BuiltinFunction is the right place for this, as that is supposed to be the base class for all functions we define in the Sage library.

I agree that extracting the original parent can be delayed. The alt_name parameter is also a good idea.

Shall I adapt my patch? or are you going to revise yours?

Changed 10 years ago by Burcin Erocal

Attachment: trac_13933-move_call.patch added

Changed 10 years ago by Burcin Erocal

Attachment: trac_13933-slow_call.patch added

robert's patch rebased on top of move_call.patch

Changed 10 years ago by Robert Bradshaw

Attachment: 13933-slow-call.v2.patch added

apply only this patch

comment:3 Changed 10 years ago by Robert Bradshaw

Status: needs_infoneeds_review

Looks like our patches crossed mid-air... I just posted a patch that moves the __call__ up.

Note that using the 3-arg getattr is better than catching an AttributeError? both for performance and so we don't mask an error during the call.

comment:4 Changed 10 years ago by Burcin Erocal

Yep, you posted your patch while I was writing this comment.

I rebased your patch on my move_call.patch from #1173, but we can just apply yours.

Here are the timings for sgn():

sage: load /home/burcin/sage/sign_fn.py
sage: sf = SignFunc()
sage: %timeit sf(5) 
625 loops, best of 3: 378 ns per loop
sage: %timeit sgn(5)                   
625 loops, best of 3: 3.49 µs per loop

Contents of the file sign_fn.py:

from sage.symbolic.function import BuiltinFunction

class SignFunc(BuiltinFunction):
    def __init__(self):
        BuiltinFunction.__init__(self, "sign")

The _alt_name code path is still taking up valuable time. It would be better to settle what name we use in the constructor and only try one option. If alt_name is set, we use that, otherwise, we use name.

There are a couple of doctest failures in sage/functions due to precision changes.

Changed 10 years ago by Robert Bradshaw

Attachment: 13933-doctests.patch added

comment:5 Changed 10 years ago by Robert Bradshaw

Description: modified (diff)

Added attachment that fixes some doctests, in each case the returned result has been changed to be of the same parent as the input.

comment:6 Changed 9 years ago by Karl-Dieter Crisman

The _alt_name code path is still taking up valuable time. It would be better to settle what name we use in the constructor and only try one option. If alt_name is set, we use that, otherwise, we use name.

So... is this needs work? Just trying to clarify what the situation is here.

Changed 9 years ago by Robert Bradshaw

Attachment: 13933-more-doctests.patch added

comment:7 Changed 9 years ago by Robert Bradshaw

Description: modified (diff)

I fixed the failed doctest and removed the old-style line continuations. Let's not let the perfect be the enemy of the good here. I disagree that the altname taking up time is a big issue--getting rid of this functionality is a much larger (and backwards incompatible) change, and for those functions without an alt name (most of them) the overhead is a single pointer comparison.

Note: See TracTickets for help on using tickets.