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.

Sign up for GitHub

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

Jump to bottom

bpo-45522: Allow to disable freelists on build time #29056

Merged
merged 5 commits into from
Oct 21, 2021
Merged

bpo-45522: Allow to disable freelists on build time #29056

merged 5 commits into from
Oct 21, 2021

Conversation

tiran
Copy link
Member

@tiran tiran commented Oct 19, 2021 •
edited by miss-islington
Loading

Freelists for object structs can now be disabled. A new configure
option --without-freelists can be used to disable all freelists
except empty tuple singleton. Internal Py*_MAXFREELIST macros can now
be defined as 0 without causing compiler warnings and segfaults.

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue45522

Automerge-Triggered-By: GH:tiran

@tiran
Copy link
Member Author

tiran commented Oct 19, 2021

  • Need to find a better way to deal with empty tuple singleton

@tiran tiran force-pushed the bpo-45522-optional-freelists branch 4 times, most recently from 8770f42 to 754b256 Compare October 19, 2021 11:49
@tiran
bpo-45522: Allow to disable freelists on build time
2af07c2 
Freelists for object structs can now be disabled. A new :program:`configure`
option :option:`!--without-freelists` can be used to disable all freelists
except empty tuple singleton.

Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran force-pushed the bpo-45522-optional-freelists branch from 754b256 to 2af07c2 Compare October 19, 2021 12:11
@tiran
Copy link
Member Author

tiran commented Oct 19, 2021

One unit test is failing when building Python --without-freelists. I assume the tests relies on freelists internally.

test test_exceptions failed -- Traceback (most recent call last):
  File "/home/heimes/dev/python/cpython/Lib/test/test_exceptions.py", line 1340, in test_recursion_normalizing_with_no_memory
    self.assertIn(b'Fatal Python error: _PyErr_NormalizeException: '
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: b'Fatal Python error: _PyErr_NormalizeException: Cannot recover from MemoryErrors while normalizing exceptions.' not found in b'sys.excepthook is missing\nobject address  : 0x7f289e86e270\nobject refcount : 1\nobject type     : 0x8212c0\nobject type name: MemoryError\nobject repr     : \nlost sys.stderr\n'

@tiran tiran requested review from corona10 and vstinner October 19, 2021 12:13
@tiran tiran marked this pull request as ready for review October 19, 2021 12:13
@tiran tiran requested review from 1st1, markshannon, methane and a team as code owners October 19, 2021 12:13
@vstinner
Copy link
Member

What is your motivation for this change? Is it an experimental change?

Freelists are important for Python performance. Shipping a Python without them doesn't seem useful.

I disabled freelists in the past when working on subinterpreters. But I modified the code in the meanwhile to make them per-interpreter.

@corona10
Copy link
Member

@tiran
Considering @vstinner 's concern(?), what about adding this package when mimalloc option is adopted?

@tiran
Copy link
Member Author

tiran commented Oct 20, 2021

What is your motivation for this change? Is it an experimental change?

Freelists are important for Python performance. Shipping a Python without them doesn't seem useful.

My motivations are listed in the ticket. tl;dr it's for alternative memory allocators like mimalloc. Mimalloc in particular has its own internal freelists system. An experimental build of Python with mimalloc and without freelists has shown minimal slown only.

If you don't like the configure option, then I can split the PR up into the internal C code fixes and the public configure option. I definitely want to land the C code changes. They also fix various bugs when Py*_MAXFREELIST are set to 0.

@tiran
Copy link
Member Author

tiran commented Oct 20, 2021

PR GH-29086 is a minimal version of this PR without the configure flag.

@corona10
Copy link
Member

@tiran I prefer this version for our future experiment :)

corona10
corona10 approved these changes Oct 20, 2021
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am okay with this PR for future experiments.
I believe that mimalloc will be adopted at least for improving performance.

see: Benchmark result which Trian and Neil digged at this sprint -> https://gist.github.com/pablogsal/8027937b71cd30f17aaaa5ef7c885d3e

@corona10 corona10 mentioned this pull request Oct 20, 2021
Closed
corona10
corona10 reviewed Oct 20, 2021
View reviewed changes
Lib/test/test_sys.py Outdated Show resolved Hide resolved
@tiran tiran mentioned this pull request Oct 20, 2021
Closed
@miss-islington miss-islington merged commit 9942f42 into python:main Oct 21, 2021
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Oct 21, 2021
@shihai1991
Merge branch 'main' into bpo_45113_3
9d162ef 
* main: (263 commits)
  bpo-45521: Fix a bug in the obmalloc radix tree code. (pythonGH-29051)
  bpo-45522: Allow to disable freelists on build time (pythonGH-29056)
  bpo-34451: Document prompt and output toggle feature in html tutorial (pythonGH-27105)
  bpo-44019: Add operator.call() to __all__ for the operator module (pythonGH-29110)
  bpo-45315: PyType_FromSpec: Copy spec->name and have the type own the memory for its name (pythonGH-29103)
  bpo-44220: Export PyStructSequence_UnnamedField in the limited API (pythonGH-26331)
  bpo-44174: [Enum] add reference to name mangling (pythonGH-29116)
  bpo-45548: add some missing entries to `Modules/Setup` (pythonGH-29115)
  bpo-35673: Add a public alias for namespace package __loader__ attribute (python#29049)
  bpo-45192: Fix a bug that infers the type of an os.PathLike[bytes] object as str (pythonGH-28323)
  bpo-45527: Don't count cache hits, just misses. (pythonGH-29092)
  bpo-45320: Remove long-deprecated inspect methods (pythonGH-28618)
  bpo-41374: Remove obsolete exclusion of netinet/tcp.h on Cygwin (pythonGH-21649)
  bpo-45532: Replace 'default' with 'main' as default in sys.version (pythonGH-29100)
  bpo-45464: [doc] Explain that subclassing multiple exceptions is fragile (pythonGH-29094)
  Cleanup a couple of comments left on PR 28775 post-merge. (pythonGH-29079)
  bpo-45536: Check OpenSSL APIs in configure (pythonGH-29088)
  Add PEPs 593 & 647 to list of PEPs at top of typing docs (pythonGH-29097)
  Add a comment about how to fix bogus test_host_resolution_bad_address failures (python#29085)
  bpo-44525: Specialize simple Python calls. (pythonGH-29033)
  ...
@tiran tiran mentioned this pull request Jun 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corona10 corona10 corona10 approved these changes

@vstinner vstinner Awaiting requested review from vstinner

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@methane methane Awaiting requested review from methane methane is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tiran @vstinner @corona10 @the-knights-who-say-ni @bedevere-bot @miss-islington