Skip to content

Conversation

@Vizonex
Copy link

@Vizonex Vizonex commented Oct 24, 2025

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_len is dropped out in favor of PyList_GET_SIZE since PyList_GET_SIZE is just a macro
  • __len__ now uses PyList_GET_SIZE() instead of _fast_len
  • instert uses *args instead of (pos, item) because insert only requires positional arguments (not keyword arguments) which ultimately saves time (many nanoseconds shaved off)
  • __iadd__ now uses PySequence_InPlaceConcat(self._items, items) instead of self._items += list(items) since there are
    less costly functions being generated by Cython and it will call a sequence function slot directly instead.
  • index only uses *args now because index can have multiple positional arguments not just item keywording these is rather slow an an ineffective approach because index only accepts positional arguments. (Feel free to let me know if the pure python version should do the same thing)
  • extend now uses just self._items.extend(items) because Cython will generate __Pyx_PyList_Extend I left a note about it as well for anyone attempting to refactor this in the future that it will not be nessesary.
  • reverse added a note for others looking to optimize this functions that Cython already generates PyList_Reverse by default
  • pop added a note that it is currently impossible to refactor and that I'm leaving that job to Cython in a future update.
  • append added a note that Cython will generate the appropriate function for self._items.append(...) (Same case with extend)
  • count Cython does a pretty sucky job at calling this function so I came up with a better apporch
    • PySequence_Count(self._items, item) PySequence has an api for handling item counting that could
      be a better approch than list.count() (Unless Cython wants to autogenerate a new function for this one in the future)
    • PyLong_FromLong to 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 uses new_list._frozen.store(True) instead of freeze() which ultimately saves another costly call.

Are there changes in behavior for the user?

Related issue number

#710

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modifications, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep the list in alphabetical order, the file is sorted by name.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .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.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@Vizonex
Copy link
Author

Vizonex commented Oct 24, 2025

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.

@Vizonex Vizonex marked this pull request as ready for review October 24, 2025 18:31
@Vizonex
Copy link
Author

Vizonex commented Oct 26, 2025

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
Copy link
Member

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):
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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.

@webknjaz webknjaz requested review from bdraco and Copilot October 28, 2025 21:32
Copy link

Copilot AI left a 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) with PyList_GET_SIZE for faster length checks
  • Updates __iadd__, extend, append, and count to use C-API or Cython-optimized implementations
  • Changes index and insert to use *args for 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.

Vizonex and others added 6 commits October 28, 2025 19:21
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants