Skip to content

Conversation

@Vizonex
Copy link
Contributor

@Vizonex Vizonex commented Jun 22, 2025

What do these changes do?

The summary of #1185 should suffice but basically the pr was accidently merged then deleted luckily I had a backup of the original code on hand so that we can resume where we left off.

Are there changes in behavior for the user?

Changes mainly will involve everything explained in #1185 but given with some better details and other suggestions from other contributors will go into this one.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 22, 2025

CodSpeed Performance Report

Merging #1190 will not alter performance

Comparing Vizonex:capi (04fb6b6) with capi (636f265)

Summary

✅ 245 untouched

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

#1185 (review) review has a lot of comments that are not applied to this PR yet.

Could you please address it, or should I move these comments to the PR?

@Vizonex

This comment has been minimized.

@Vizonex

This comment has been minimized.

@Vizonex

This comment has been minimized.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 23, 2025

Tomorrow Or Later I will see about tracking down the gc/weakref bug.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 24, 2025

@asvetlov I'm back. I woke up. I ran pre-commit install inside my env.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 24, 2025

@asvetlov When I run pre-commit run --all-files here's what I get

Validate GitHub Workflows.......................................................Passed
Check GitHub Workflows set timeout-minutes......................................Passed
Validate ReadTheDocs Config.....................................................Passed
yamllint........................................................................Failed
- hook id: yamllint
- exit code: 1

.clang-format
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/workflows/ci-cd.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/FUNDING.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/dependabot.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.codecov.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.pre-commit-config.yaml
  1:4       error    wrong new line character: expected \n  (new-lines)

.readthedocs.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/workflows/reusable-cibuildwheel.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/workflows/auto-merge.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/config.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.yamllint
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/workflows/codeql.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

.github/workflows/reusable-linters.yml
  1:4       error    wrong new line character: expected \n  (new-lines)

ruff check......................................................................Passed
ruff format.....................................................................Passed
there must be not top-level `__init__.py` in `tests/`.......(no files to check)Skipped
changelog filenames.........................................(no files to check)Skipped
Changelog files should use a non-broken :user:`name` role.......................Passed
clang-format....................................................................Passed
MyPy, for Python 3.13...........................................................Passed
MyPy, for Python 3.11...........................................................Passed

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 24, 2025

Ok so it appears all I have left is documentation according to the linter this should be an easy one to fix seems I had some spelling errors when I was writing late at night.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 24, 2025

@asvetlov It's failing to built the wheels for mac-os for some strange reason but I will wait for you to get back before I do anything else...

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 24, 2025

@asvetlov Workflow seems to be ignore testcapi but I'm sure you'll find a way to get it to run.

@codecov
Copy link

codecov bot commented Jun 24, 2025

Codecov Report

Attention: Patch coverage is 69.91525% with 71 lines in your changes missing coverage. Please review.

Project coverage is 96.12%. Comparing base (bfee715) to head (51bbc70).

Files with missing lines Patch % Lines
tests/test_capi.py 69.91% 69 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (0.43%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project check has failed because the head coverage (88.37%) is below the target coverage (100.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             capi    #1190      +/-   ##
==========================================
- Coverage   97.57%   96.12%   -1.46%     
==========================================
  Files          28       28              
  Lines        4125     4335     +210     
  Branches      736      738       +2     
==========================================
+ Hits         4025     4167     +142     
- Misses         52      119      +67     
- Partials       48       49       +1     
Flag Coverage Δ
CI-GHA 96.12% <69.91%> (-1.46%) ⬇️
MyPy 79.62% <69.91%> (-0.62%) ⬇️
pytest 91.02% <0.43%> (-5.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vizonex Vizonex requested a review from asvetlov June 24, 2025 21:18
@asvetlov
Copy link
Member

Sorry, this evening I was busy preparing the 6.5.1 bugfix release.
Please let me review tomorrow.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 24, 2025

So I have a theory on what caused MultiDict_New(...) to crash in the first place.
While playing around for a bit I discovered that using tp_alloc(XXX, 0) doesn't crash vs using the PyObject_GC_New method like you had before.

PyObject_GC_New(MultiDictObject, state->MultiDictType);

The problem may be tied to how Multidict is setup to first allocate memory.
{Py_tp_new, PyType_GenericNew},

Looking at PyType_GenericNew a bit deeper it turns out to be just doing this under the hood.

PyObject *
PyType_GenericNew(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
    return type->tp_alloc(type, 0);
}

Replacing PyObject_GC_New with PyType_GenericNew or type->tp_alloc(type, 0) prevents the crash.
So what I am going to do for now is replace what I have with it after we run the capi-tests first on all versions and systems and then we can try this incase the problem is somehow version related.

Sorry, this evening I was busy preparing the 6.5.1 bugfix release. Please let me review tomorrow.

My bad I'll wait for tomorrow, I apologize.

@asvetlov
Copy link
Member

asvetlov commented Jun 25, 2025

Replacing PyObject_GC_New with PyType_GenericNew or type->tp_alloc(type, 0) prevents the crash.

Yes, you are right. Better to call tp_alloc slot than PyType_GenericNew to prevent the case of changing tp_alloc slot in the future.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 25, 2025

@asvetlov I'll leave the rest to you now, now that I've replaced the GC_New with tp_alloc All we need now is for the workflows to run the capi tests and will be all set to merge and then work on the rest of the c-api.

@asvetlov
Copy link
Member

Ok.
The only thing that I'm not sure about is MultiDict_Equals.
In CPython, PyObject_RichCompare is used everywhere in public API, only PyUnicode has own comparison functions.
I doubt if we need MultiDict_Equals; a shorter API is better. PyObject_RichCompare should have almost the same performance as MultiDict_Equals; its usage is relatively unfrequent, I guess.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 25, 2025

Ok. The only thing that I'm not sure about is MultiDict_Equals. In CPython, PyObject_RichCompare is used everywhere in public API, only PyUnicode has own comparison functions. I doubt if we need MultiDict_Equals; a shorter API is better. PyObject_RichCompare should have almost the same performance as MultiDict_Equals; its usage is relatively unfrequent, I guess.

Alright, makes sense I'll see about removing this one here in a bit.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 26, 2025

@asvetlov I'm going to see about resolving a few conflicts today with this repo and test_multidict_benchmarks and then maybe after that we can see about merging and then we can work on adding in the rest of the api soon. Know that I will not be available on Saturday I may have a ton of stuff happening that day. After that, we should be fine.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 26, 2025

@asvetlov I'll make sure your done with your benchmarks pr and then I will proceed.

@asvetlov
Copy link
Member

I've done, benchmarks are stable now.

@Vizonex
Copy link
Contributor Author

Vizonex commented Jun 26, 2025

@asvetlov Sounds good let me sync what I need to sync

@Vizonex
Copy link
Contributor Author

Vizonex commented Oct 1, 2025

@asvetlov If I'm going to be honest maybe we should have the testcapi module in the multidict library as an underscore module as something like multidict._testcapi so that we don't have to waste anymore time with trying to play a game of "try brute-forcing this in and will it work?" Did you have any other ideas on compiling testcapi with the workflow that I didn't try yet?

@Vizonex
Copy link
Contributor Author

Vizonex commented Oct 2, 2025

I have to get off to work today but I should be back in 7-8 hours but if anybody wants to try and disabling 3.13t from trying to install cffi and seeing if testcapi passes on all other systems and then merge the pull-request if it all of it passes be my guest.

@Vizonex
Copy link
Contributor Author

Vizonex commented Oct 3, 2025

@asvetlov I came up with a temporary solution for CFFI 2.0.0 on 3.13t, I'll revert the workflow script when it's done but just know this can come back to bite us later if were not careful with how this hack is used...

cffi == 1.17.1; python_version=="3.13" # CFFI-2.0.0 breaks workflows under 3.13t

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants