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

dlist is no more used since 15b3b3f5 #3048

Merged
merged 13 commits into from
Aug 29, 2024
Merged

dlist is no more used since 15b3b3f5 #3048

merged 13 commits into from
Aug 29, 2024

Conversation

alkino
Copy link
Member

@alkino alkino commented Aug 15, 2024

No description provided.

Copy link

✔️ a9ba274 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Base automatically changed from cornu/nanobind_loads_dumps to master August 15, 2024 12:07
Copy link

✔️ af6df4f -> Azure artifacts URL

Copy link

codecov bot commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.26%. Comparing base (2996006) to head (8f82083).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3048      +/-   ##
==========================================
- Coverage   67.27%   67.26%   -0.01%     
==========================================
  Files         571      571              
  Lines      104877   104867      -10     
==========================================
- Hits        70554    70541      -13     
- Misses      34323    34326       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

@1uc
Copy link
Collaborator

1uc commented Aug 19, 2024

I'll summarize my understanding:

  1. Prior to Py2NRNString we needed to keep certain Python strings alive while the HOC string was alive.
  2. The solution was to defer decrementing the counter on those Python strings until later.
  3. The dlist was where we put then, while they awaited their decref.
  4. The Py2NRNString immediately made a copy, therefore the Python string and HOC string had separate lifetimes.
  5. We continued to defer the decref on the Python string; but stopped decref'ing them. Effectively causing the Python strings to leak.

This PR fixes the leak by removing the code to defer decref'ing. @alkino is that a sensible summary of what's happening?

@1uc
Copy link
Collaborator

1uc commented Aug 29, 2024

sensible summary of what's happening?

No, they're not leaked. It's just removing dead code.

Copy link

sonarcloud bot commented Aug 29, 2024

Copy link

✔️ 8f82083 -> Azure artifacts URL

@1uc 1uc merged commit b985a12 into master Aug 29, 2024
38 checks passed
@1uc 1uc deleted the cornu/no_more_dlist branch August 29, 2024 11:34
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