Skip to content

Refactor getattrbehavior to avoid allocations#224

Merged
MatthieuDartiailh merged 2 commits intonucleic:mainfrom
frmdstryr:optimize-getattrbehavior
Jan 20, 2025
Merged

Refactor getattrbehavior to avoid allocations#224
MatthieuDartiailh merged 2 commits intonucleic:mainfrom
frmdstryr:optimize-getattrbehavior

Conversation

@frmdstryr
Copy link
Contributor

Same as #223 but separate PR to make it easier to review.

@frmdstryr
Copy link
Contributor Author

frmdstryr commented Jan 18, 2025

I think if catom is updated to use the fastcall protocol more optimizations can be done here (with created_args).

@frmdstryr
Copy link
Contributor Author

frmdstryr commented Jan 18, 2025

Could the mangled name _get_%s somehow be cached instead of reformatting it every time?

@sccolbert
Copy link
Member

Could the mangled name _get_%s somehow be cached instead of reformatting it every time?

Can you link to a line number where you mean?

@frmdstryr
Copy link
Contributor Author

frmdstryr commented Jan 18, 2025

Here https://github.com/nucleic/atom/blob/main/atom/src/getattrbehavior.cpp#L161

Idk if the getattr_context can be repurposed to store the name?

Edit: Or better yet the method itself

@sccolbert
Copy link
Member

sccolbert commented Jan 18, 2025

I don't think that's worth the tradeoff. The Property members aren't used very often, and while they could benefit from some more storage for caching, that would complicate the entire Member struct, which tries to be as space-efficient as possible.

When I originally wrote atom, it was for a commercial purpose sponsored by my employer. In that setting, optimizing memory use was the main driver. They have a very large codebase with tens of millions of Python objects instantiated at any given time. Since they are still using the library AFAIK, I don't want to make changes that would explode memory usage for them.

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.

This is a very nice diff !

@MatthieuDartiailh
Copy link
Member

I will restart the CIs later (the API rate limits for external contributor is so annoying) and merge when they come back green.

@MatthieuDartiailh MatthieuDartiailh merged commit 32bedfe into nucleic:main Jan 20, 2025
18 checks passed
@codecov
Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.67%. Comparing base (1dc853d) to head (2b59255).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #224   +/-   ##
=======================================
  Coverage   97.67%   97.67%           
=======================================
  Files          24       24           
  Lines        1074     1074           
  Branches      162      162           
=======================================
  Hits         1049     1049           
  Misses         12       12           
  Partials       13       13           

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