Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#13933 closed defect (fixed)

BuiltinFunction.__call__ is unnecessarily slow

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

Description (last modified by burcin)

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:

Attachments (6)

13933-slow-call.patch (5.3 KB) - added by robertwb 5 years ago.
Any reason why this isn't a good idea?
trac_13933-move_call.patch (5.4 KB) - added by burcin 5 years ago.
trac_13933-slow_call.patch (3.6 KB) - added by burcin 5 years ago.
robert's patch rebased on top of move_call.patch
13933-slow-call.v2.patch (6.7 KB) - added by robertwb 5 years ago.
apply only this patch
13933-doctests.patch (3.5 KB) - added by robertwb 5 years ago.
13933-more-doctests.patch (2.7 KB) - added by robertwb 4 years ago.

Download all attachments as: .zip

Change History (21)

Changed 5 years ago by robertwb

Any reason why this isn't a good idea?

comment:1 Changed 5 years ago by robertwb

  • Cc burcin added
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by burcin

  • Authors set to Robert Bradshaw
  • Reviewers set to Burcin Erocal
  • Status changed from needs_review to needs_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 5 years ago by burcin

Changed 5 years ago by burcin

robert's patch rebased on top of move_call.patch

Changed 5 years ago by robertwb

apply only this patch

comment:3 Changed 5 years ago by robertwb

  • Status changed from needs_info to needs_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 5 years ago by burcin

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 5 years ago by robertwb

comment:5 Changed 5 years ago by robertwb

  • 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 4 years ago by kcrisman

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 4 years ago by robertwb

comment:7 Changed 4 years ago by robertwb

  • 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.

comment:8 Changed 4 years ago by kcrisman

I would be likely to agree with this sentiment. Burcin, are you okay with Robert's main patch in that case, as it seems? Then we can just look at the doctests and make sure that was all of the fixes.

I'm not clear on what would be backwards-incompatible, though?

comment:9 Changed 4 years ago by robertwb

It's backwards incompatible because if some objects (defined within or outside of Sage) define sgn() and others define sign(), removing this aliasing would cause one or the other break.

If we decide to go this route, I think it should be a separate ticket than this performance enhancement.

comment:10 Changed 4 years ago by kcrisman

Okay, you're not saying that anything in this ticket is breaking anything. I didn't think so...

comment:11 Changed 4 years ago by burcin

  • Description modified (diff)
  • Keywords sd48 added
  • Reviewers changed from Burcin Erocal to Burcin Erocal, Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

comment:12 Changed 4 years ago by kcrisman

Patchbot, apply 13933-slow-call.v2.patch 13933-doctests.patch 13933-more-doctests.patch

comment:13 Changed 4 years ago by kcrisman

All tests pass.

comment:14 Changed 4 years ago by jdemeyer

  • Merged in set to sage-5.11.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 3 years ago by kcrisman

Hi guys! This seems to have either

  • exposed inadequate case-checking in trig.py, or
  • contained a bug for functions which are BuiltinFunctions but used to be directly evaluated using Function's __call__ method, as opposed to the one now in BuiltinFunction (which used to only be used for GinacFunction).

Can one of you take a look at comment:31:ticket:13246 to help us ascertain which one it is? Thanks!

Note: See TracTickets for help on using tickets.