-
-
Notifications
You must be signed in to change notification settings - Fork 33
Refractor some of FrozenList's functions and methods #712
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
base: master
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
|
I'll handle other typos a bit later but for now lets wait for the workflows to pass. Once the workflows pass I will mark this Pull Request as ready for review. |
|
Perhaps maybe a rerun with some of the stuff I've added will help. I will be a bit busy for today so I may look into this a bit more tomorrow. |
| @@ -0,0 +1,2 @@ | |||
| Refractor methods `__len__`, `__iadd__`, `index`, `extend`, `append`, `count` and `__deecopy__` and add notes for `pop`, `append` and `extend` for contributors who are looking to | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use RST, not markdown.
| return list(self) >= other | ||
|
|
||
| def insert(self, pos, item): | ||
| def insert(self, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a bad idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with insert is that it only does positional arguments. Hand-passing them off seemed to be the faster move.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you're trying to achieve but here's a typical signature: https://docs.python.org/3/library/stdtypes.html#sequence.insert
We shouldn't have differing signatures when we mimic interfaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors several methods in the FrozenList class to use direct C-API calls for improved performance. The changes replace Python method calls with Cython-optimized equivalents and add inline comments explaining the optimization strategies.
- Replaces
len(self._items)withPyList_GET_SIZEfor faster length checks - Updates
__iadd__,extend,append, andcountto use C-API or Cython-optimized implementations - Changes
indexandinsertto use*argsfor proper argument forwarding - Optimizes
__deepcopy__to call._frozen.store(True)directly instead of.freeze()
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| frozenlist/_frozenlist.pyx | Refactors multiple methods to use C-API calls and Cython optimizations for better performance |
| CHANGES/712.feature.rst | Adds changelog entry describing the refactored methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
What do these changes do?
NOTE: Ignore the name of the branch, I originally planned on using a C++ vector but then I figured out quickly that would be a bad approach due to memory allocations being mixed.
Instead of trying to migrate the Code on over to C I decided to instead try refactoring some functions that Cython has problems with generating better code for. I also added in a few notes and highlights for anyone else planning to refactor some of these functions later in the near future and a special note that pop is currently unoptimizable unless Cython wants to brainstorm a better solution in a later update.
(Please note that I am using cython version 3.1.6 and that it was a more recent update and the optimizations may not be the same if generated with 3.1.4`)
The following items have been changed
If asked by a maintainer to put all these changes into a changes.rst I will gladly try doing all by myself and run precommit locally so even if getting everything into it is stressful enough I'm not brute-forcing in too many commits.
_fast_lenis dropped out in favor ofPyList_GET_SIZEsincePyList_GET_SIZEis just a macro__len__now usesPyList_GET_SIZE()instead of_fast_len*argsinstead of (pos, item) because insert only requires positional arguments (not keyword arguments) which ultimately saves time (many nanoseconds shaved off)__iadd__now usesPySequence_InPlaceConcat(self._items, items)instead ofself._items += list(items)since there areless costly functions being generated by Cython and it will call a sequence function slot directly instead.
indexonly uses*argsnow because index can have multiple positional arguments not justitemkeywording these is rather slow an an ineffective approach becauseindexonly accepts positional arguments. (Feel free to let me know if the pure python version should do the same thing)extendnow uses justself._items.extend(items)because Cython will generate__Pyx_PyList_ExtendI left a note about it as well for anyone attempting to refactor this in the future that it will not be nessesary.reverseadded a note for others looking to optimize this functions that Cython already generates PyList_Reverse by defaultpopadded a note that it is currently impossible to refactor and that I'm leaving that job to Cython in a future update.appendadded a note that Cython will generate the appropriate function forself._items.append(...)(Same case withextend)countCython does a pretty sucky job at calling this function so I came up with a better apporchPySequence_Count(self._items, item)PySequence has an api for handling item counting that couldbe a better approch than
list.count()(Unless Cython wants to autogenerate a new function for this one in the future)PyLong_FromLongto save a step in needing to handle important conversions since long and int are almost identical in types. (Feel free to correct or criticize me if I'm wrong about this)__deepcopy__now usesnew_list._frozen.store(True)instead offreeze()which ultimately saves another costly call.Are there changes in behavior for the user?
Related issue number
#710
Checklist
CONTRIBUTORS.txtCHANGESfolder<issue_id>.<type>for example (588.bugfix)issue_idchange it to the pr id after creating the pr.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.