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

Move standard_tracer's subscription observer to cpp #562

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

frmdstryr
Copy link
Contributor

@frmdstryr frmdstryr commented Jan 6, 2025

Moves the subscription observer in standard_tracer.py to cpp.

This gives a considerable performance increase if there are a lot of changes going on (~60%) but for most code it will likely only be a small improvement. My application sped up about 30% (even after fixing the original problem).

Ref #561

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.91%. Comparing base (3bcd2a4) to head (2793fa8).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #562      +/-   ##
==========================================
- Coverage   70.92%   70.91%   -0.01%     
==========================================
  Files         287      287              
  Lines       25710    25698      -12     
  Branches     3643     3641       -2     
==========================================
- Hits        18234    18224      -10     
  Misses       6385     6385              
+ Partials     1091     1089       -2     

@frmdstryr frmdstryr force-pushed the subscription-observer-in-cpp branch from 27b4454 to 51e0a8f Compare January 6, 2025 21:12
Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

I made several suggestions to simplify your handling of reference counts. I tested locally and it works but feel free to give it another spin.

Could you also add a changelog entry ?

enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
@frmdstryr
Copy link
Contributor Author

Thanks for the review.

I tried moving the whole standard_tracer to cpp here but this branch actually faster somehow so I think it's better to just go with this branch as it's also much simpler.

@MatthieuDartiailh
Copy link
Member

Can you rebase on main to get the CI to pass ?

@MatthieuDartiailh
Copy link
Member

I tried moving the whole standard_tracer to cpp here but this branch actually faster somehow so I think it's better to just go with this branch as it's also much simpler.

This is unexpected but I will take your word for it. No reason to add more cpp if there is no real gain.

@frmdstryr frmdstryr force-pushed the subscription-observer-in-cpp branch from be40300 to dda8fa8 Compare January 7, 2025 18:38
@frmdstryr
Copy link
Contributor Author

frmdstryr commented Jan 7, 2025

I was able to get the poor example in #561 even faster by moving all the PyImport_ImportModule into the modexec (from ~16sec to ~12sec).

@sccolbert
Copy link
Member

sccolbert commented Jan 7, 2025 via email

@MatthieuDartiailh
Copy link
Member

MatthieuDartiailh commented Jan 7, 2025

Sure, @sccolbert , I will wait for your approval before merging.

enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Show resolved Hide resolved
enaml/src/subscription_observer.cpp Show resolved Hide resolved
enaml/src/subscription_observer.cpp Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
enaml/src/subscription_observer.cpp Outdated Show resolved Hide resolved
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.

3 participants