Skip to content

Conversation

Bobronium
Copy link
Contributor

@Bobronium Bobronium commented Oct 31, 2020

Change Summary

Creation time:

  • Use sys._getframe() instead of slow inspect.stack()
  • Use frame.f_globals instead of slow inspect.getmodule() to retrieve caller module name
  • Combine get_caller_module_name() with is_call_from_module() into more than 2000x 🙈 faster get_caller_frame_info()

Concrete names reusing:

  • Append '_' to global reference name if it's already in use instead of raising TypeError

Related issue number

fix #2070
fix #2069

(updated: added "fix" so I don't forget to close both issues, @samuelcolvin)

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

combine get_caller_module_name and is_call_from_module in get_caller_frame_info
@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #2078 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2078   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           21        21           
  Lines         4094      4091    -3     
  Branches       821       822    +1     
=========================================
- Hits          4094      4091    -3     
Impacted Files Coverage Δ
pydantic/generics.py 100.00% <100.00%> (ø)
pydantic/fields.py 100.00% <0.00%> (ø)
pydantic/decorator.py 100.00% <0.00%> (ø)
pydantic/dataclasses.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95435de...fe88e7f. Read the comment docs.

f'Name conflict: {model_name!r} in {model_module!r} is already used by {object_in_module!r}'
)
if called_globally: # create global reference and therefore allow pickling
object_by_reference = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for also taking care of this issue too besides performance one! 🎉

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs to be added in v1.7.2 once last tests to keep coverage at 100% are added and @samuelcolvin approves

@Bobronium
Copy link
Contributor Author

Bobronium commented Oct 31, 2020

Coverage should be fixed now

Here's a benchmark showcasing the difference in speed between 1.7.1, 1.6.x and current PR:
https://repl.it/repls/HorizontalConcernedWatchdog#main.py

@bogdandm, could you run this one in your environment as well?

@Bobronium
Copy link
Contributor Author

Bobronium commented Oct 31, 2020

Just decided to compare speed of calling get_caller_frame_info() vs get_caller_module_name() with is_call_from_module():

import sys
from timeit import timeit

def get_caller_frame_info_new():
    previous_caller_frame = sys._getframe(2)
    frame_globals = previous_caller_frame.f_globals
    return frame_globals.get('__name__'), previous_caller_frame.f_locals is frame_globals

def get_caller_frame_info_old():
    # code of two functions placed in one and results are combined
    import inspect

    previous_caller_frame = inspect.stack()[2].frame
    previous_caller_module = inspect.getmodule(previous_caller_frame, previous_caller_frame.f_code.co_filename)
    name = previous_caller_module.__name__ if previous_caller_module is not None else None

    import inspect
    previous_caller_frame = inspect.stack()[2].frame
    called_globally = previous_caller_frame.f_locals is previous_caller_frame.f_globals

    return name, called_globally

def check():
    assert get_caller_frame_info_new() == get_caller_frame_info_old()

check()
old = timeit(get_caller_frame_info_old, number=1000)
new = timeit(get_caller_frame_info_new, number=1000)
print('new: ', new)
print('old: ', old)
print('old / new: ', old / new)

I'm getting this results

new:  0.00040132100000001003
old:  1.176067545
old / new:  2930.490916249014

Three. Thousand. Times. Faster. ON SSD. 🙈

I don't know what to say. Previous implementation is the definition of something unreasonably slow...

@samuelcolvin
Copy link
Member

This is looking great, I'm going to play with it myself but it looks good.

Would also be good to wait for confirmation from @bogdandm.

@samuelcolvin
Copy link
Member

Once this is merged, I'll release v1.7.2. If there's anything else we should include, let me know.

@bogdandm
Copy link

bogdandm commented Oct 31, 2020

@MrMrRobat Could confirm that it is working (similar HDD setup, but a little bit faster disk I think)

new:  0.0005787028931081295
old:  1.042893337085843
old / new:  1802.1222107334108

And 2000 times faster on SSD as well.
Also just checked it by installing via pip from commit hash - it is as fast as 1.5.1 with my code example from the issue.

@samuelcolvin samuelcolvin merged commit 4a09447 into pydantic:master Oct 31, 2020
@samuelcolvin
Copy link
Member

Awesome. I'll make a new release in the morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[perfomance regression] Generic model creation cause huge slowdown Repeatedly subclassing a GenericModel with a Constrained Type fails
4 participants