classification
Title: Obmalloc lock LOCK_INIT and LOCK_FINI are never used
Type: behavior Stage: needs patch
Components: Interpreter Core Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: larry Nosy List: larry, pitrou, tim.peters, vstinner
Priority: low Keywords: patch

Created on 2016-04-14 07:55 by larry, last changed 2016-04-16 08:43 by larry.

Files
File name Uploaded Description Edit
larry.refresh.lock.macros.for.obmalloc.diff larry, 2016-04-16 08:43 review
Messages (6)
msg263377 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-04-14 07:55
Obmalloc now has theoretical support for locking.  I say theoretical because I'm not convinced it's ever been used.

The interface is defined through five macros:
    SIMPLELOCK_DECL
    SIMPLELOCK_INIT
    SIMPLELOCK_FINI
    SIMPLELOCK_LOCK
    SIMPLELOCK_UNLOCK

Internally these are used to define an actual lock to be used in the module.  The lock, "_malloc_lock", is declared, then four defines are made building on top of the SIMPLELOCK macros, named:
    LOCK
    UNLOCK
    LOCK_INIT
    LOCK_FINI

LOCK_INIT and LOCK_FINI are never called.  So unless your lock doesn't happen to require initialization or shutdown, this API is misimplemented.

Victor: this was your work, right?  If not, sorry, please unassign/de-nosy yourself.
msg263381 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-04-14 08:34
These macros are very old, I didn't write them. They are not used since the
API rely on the GIL.

Do you want to remove them?

I think that it's ok to keep them, just in case, if tomorrow we want to
support multiple allocations in parallel. Maybe a comment should explain
that these macros are not used because of the GIL.
msg263382 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-04-14 08:38
There's already a comment saying that the macros are empty because the GIL protects obmalloc from parallelization.

I'd be happy to improve the code and add calls for LOCK_INIT and LOCK_FINI in the proper places.  You don't have to do it.  Since this isn't your goof I'll go ahead and own it.
msg263441 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2016-04-15 04:01
Right, these macros were in the original module (by Vladimir Marangozov).  They've never done anything - never been tested.  Over the years I removed other layers of macro indirection (while other people added more ;-) ), but left these alone because they point out at least some speed-crucial places where "removing the GIL" would add new costs.

That said, I wouldn't object if you removed all the lock-related macros in obmalloc.  The code is hairier now than it was at the start, so throwing out unused cruft is also more valuable now than it was at the start.
msg263541 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-04-16 08:43
Patch attached.  The basics were okay; however, there was no locking around access to a static variable (_Py_AllocatedBlocks) so I added some.  The way the code managed _Py_AllocatedBlocks was a bit odd; this approach resulted in fewer lines, but it was hard to follow, and adding locking support would have muddied it even further, so I simplified it.  I also simplified the locking support a great deal ("SIMPLELOCK": YAGNI) and touched up the relevant comment.

Finally, I noticed a minor bug wrt added _Py_AllocatedBlocks: if you call PyMem_Realloc(NULL, 50000), that's really a new allocation, so _Py_AllocatedBlocks needs to be incremented, but it wasn't.  Since Antoine is the father of _Py_AllocatedBlocks I added him to the nosy list.
msg263542 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2016-04-16 08:43
Oy veh, in editing the issue I dropped the attached file.  Here it is.
History
Date User Action Args
2016-04-16 08:43:49larrysetfiles: + larry.refresh.lock.macros.for.obmalloc.diff
keywords: + patch
messages: + msg263542
2016-04-16 08:43:10larrysetnosy: + pitrou
messages: + msg263541
2016-04-15 04:01:19tim.peterssetnosy: + tim.peters
messages: + msg263441
2016-04-14 08:38:49larrysetassignee: vstinner -> larry
messages: + msg263382
2016-04-14 08:34:13vstinnersetmessages: + msg263381
2016-04-14 07:55:46larrycreate