GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-105766: Add Locking Around Custom Allocators by ericsnowcurrently · Pull Request #105619 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Jun 10, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The "mem" and "object" allocators are documented as dependent on the GIL. However, after per-interpreter GIL landed we weren't enforcing that. This fixes that by using wrapping all allocations/frees with a dedicated runtime-global lock, but only if necessary. Note that we actually use the main interpreter's GIL as that lock, but only for interpreter's that have their own GIL. Doing so closely matches the original behavior.

  • Issue: "mem" and "object" Allocators are No Longer Protected by the GIL #105766

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ericsnowcurrently added 4 commits June 8, 2023 14:16
@ericsnowcurrently
Add wrapper allocators with locking when needed.
fcbeb6d
@ericsnowcurrently
Fix set_allocator_unlocked.
fbe26eb
@ericsnowcurrently
Implement should_lock().
7191629
@ericsnowcurrently
Implement acquire_custom_allocator_lock() and release_custom_allocato…
dd60989 
…r_lock().
@ericsnowcurrently
ericsnowcurrently requested review from markshannon and vstinner June 10, 2023 00:32
@bedevere-bot bedevere-bot mentioned this pull request Jun 10, 2023
Improve Interpreter Isolation #100227
Closed
@ericsnowcurrently ericsnowcurrently added skip news needs backport to 3.12 only security fixes labels Jun 10, 2023
@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

@vstinner, any objections? I tried to do this in a way that would not penalize the main interpreter or interpreters that share the GIL. Likewise, allocators that only wrap the current allocator should not be affected.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon

markshannon commented Jun 12, 2023

Copy link
Copy Markdown
Member

The rules was (is?) that you need to hold the GIL to call PyObject_Malloc, etc.
So why do we now need any locks (apart from the GIL)?

Presumably, the presence of per-interpreter GILs, means that allocators need to use per-interpreter state rather than process-wide state, but I don't see how this enforces that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

The allocators are process-global, so a per-interpreter GIL wouldn't guard against races between interpreters. Requiring that they be per-interpreter would mean we'd have to change the allocator API.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
ericsnowcurrently marked this pull request as ready for review June 12, 2023 15:50
@bedevere-bot bedevere-bot added the awaiting core review label Jun 12, 2023
@markshannon

markshannon commented Jun 12, 2023

Copy link
Copy Markdown
Member

If the allocators are process-wide, there would need to be a lock on every call to ob_malloc, which would be disastrous for performance. So that doesn't sound right
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

Yeah, we lock around every allocation when a custom, non-wrapper allocator is used in a subinterpreter that has its own GIL. I don't see what else we can do, aside from doing nothing (allowing races).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2023

Copy link
Copy Markdown
Member Author

Maybe we just ask custom allocators to make sure they are thread-safe?
markshannon and gpshead reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently ericsnowcurrently mentioned this pull request Jun 14, 2023
"mem" and "object" Allocators are No Longer Protected by the GIL #105766
Closed
@ericsnowcurrently ericsnowcurrently changed the title gh-100227: Add Locking Around Custom Allocators gh-105766: Add Locking Around Custom Allocators Jun 14, 2023
gpshead
gpshead requested changes Jul 27, 2023
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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.

overall yes I think something like this PR is needed for the current state of affairs. reusing the main runtime's GIL as the allocator lock is also what I'd assumed would be a natural first implementation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/pylifecycle.c Outdated
return status;
}
HEAD_LOCK(runtime);
runtime->allocators.num_gils++;

@gpshead gpshead Jul 26, 2023

Copy link
Copy Markdown
Member

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.

Check for overflow. if the value is already INT_MAX pre-increment, we need to bail, even if that means SystemError.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/pylifecycle.c Outdated

_PyRuntimeState *runtime = interp->runtime;
HEAD_LOCK(runtime);
runtime->allocators.num_gils--;

@gpshead gpshead Jul 26, 2023

Copy link
Copy Markdown
Member

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.

Add an assert(runtime->allocators.num_gils > 0); before this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Include/internal/pycore_pymem.h Outdated
debug_alloc_api_t obj;
} debug;
PyObjectArenaAllocator obj_arena;
int num_gils;

@gpshead gpshead Jul 26, 2023

Copy link
Copy Markdown
Member

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 suggest unsigned int

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/obmalloc.c
_PyMem_MallocLocked(void *ctx, size_t size)
{
PyMemAllocatorEx *wrapped = (PyMemAllocatorEx *)ctx;
if (_PyRuntime.allocators.num_gils > 1) {

@gpshead gpshead Jul 27, 2023

Copy link
Copy Markdown
Member

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.

You lock updates (writes) to this value. Is it safe to use without atomic access or a lock? (requiring an atomic read here would presumably be a performance hit?).

Rather than always checking... could the logic that does num_gils++ with the (main runtime) lock held also do:

... num_gils++;
if (num_gils == 2 && !has_locking_wrapper(...)) {
    maybe_add_locking_wrapper(...);
}

such that the wrapped pointer switch happens upon first creation of an additional gil while the main runtime gil (the only gil prior to the current code running) is still held.

I'm not sure it is worth doing the opposite during finalization. Once wrapped due to per subinterpreter GILs, just stay wrapped. At least the wrappers won't have this conditional anymore.

I suspect this thought is either overoptimization... or actually necessary to avoid locking/atomic access to num_gils.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jul 27, 2023

Copy link
Copy Markdown
Member Author

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.

That's a great idea. I'll try it out.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting core review label Jul 27, 2023
@bedevere-bot

bedevere-bot commented Jul 27, 2023

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jul 27, 2023
@gpshead

gpshead commented Jul 27, 2023

Copy link
Copy Markdown
Member

note the discussion Eric started: https://discuss.python.org/t/what-to-do-about-custom-allocators-and-the-gil/30327
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently added 4 commits July 27, 2023 14:21
@ericsnowcurrently
Merge branch 'main' into lock-around-custom-allocators
0f2a242
@ericsnowcurrently
Check for overflow/underflow.
321d1c2
@ericsnowcurrently
Use an unsigned int.
52c01ea
@ericsnowcurrently
Only modify num_gils if the interpreter has its own GIL.
a213597
@ericsnowcurrently

ericsnowcurrently commented Jul 28, 2023

Copy link
Copy Markdown
Member Author

I'm closing this. Instead, we'll specify that allocators must be thread-safe (at least when isolated subinterpreters are in play).
gpshead reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently ericsnowcurrently closed this Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead requested changes
@vstinner vstinner Awaiting requested review from vstinner
@markshannon markshannon Awaiting requested review from markshannon

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes needs backport to 3.12 only security fixes skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ericsnowcurrently @markshannon @bedevere-bot @gpshead
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.