Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster $RecursionLimit #501

Closed
wants to merge 2 commits into from
Closed

Conversation

poke1024
Copy link
Contributor

@poke1024 poke1024 commented Aug 25, 2016

The currrent check for $RecursionLimit is slow for functions with recursion. For these cases, this PR yields a 20% improvement in performance.

This PR caches $RecursionLimit in an attribute of Definitions, thus avoiding having to look it up in the symbol tables. As the setting of $RecursionLimit already has special case inassignment.py, this change is easy. EDIT this turned out to be not true.

Before:

In[1]:= Timing[Length[#+1& /@ Range[10000]]]
Out[1]= {5.00666, 10000}

After:


In[2]:= Timing[Length[#+1& /@ Range[10000]]]
Out[2]= {4.08788, 10000}

Most other functions are not changed in their performance:

before.txt

after.txt

@sn6uv
Copy link
Member

sn6uv commented Aug 25, 2016

I wonder if there's a better way to do this. Is it actually the dictionary lookup that is slow? Could we just cache the Definition object for $RecursionLimit to avoid all the additional self.recursionlimit = None lines?

update: tried this but it didn't work. I'm guessing a new Definition object is being created.

@poke1024
Copy link
Contributor Author

I don't completely understand why it takes so long, but I keep seeing lookup_name popping up in my performance profiles with quite big numbers and I suspect it has to do with the long chain needed to resolve a symbol, i.e.

Definitions.get_config_value()
which calls Definitions.get_definition()
which calls Definitions.lookup_name()
which calls Definitions.get_current_context()
which calls Definitions.get_ownvalue('System`$Context')
which calls Definitions.get_definition()
which calls Definitions.lookup_name()

I agree that the self.recursionlimit = None lines are ugly. Maybe I'll delegate them into an ownvalues_changed or something?

Again, probably this is not the right approach and $RecursionLimit is just the tip of the iceberg. The real problem might be that Definitions.get_definition() needs to be really fast. I looked at this code quite some time now, but it seems really hard to add some general caching there.

@poke1024 poke1024 mentioned this pull request Aug 25, 2016
@poke1024
Copy link
Contributor Author

I'm continuing this with a more generic approach in #507.

@poke1024 poke1024 closed this Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants