Bug 242274

Summary: lang/python{27,35,36,37,38}: Add closefrom(2) support
Product: Ports & Packages Reporter: Kyle Evans <kevans>
Component: Individual Port(s)Assignee: Kubilay Kocak <koobs>
Status: Closed FIXED    
Severity: Affects Only Me CC: jonaspalm, python
Priority: --- Flags: koobs: merge-quarterly-
Version: Latest   
Hardware: Any   
OS: Any   
Bug Depends on:    
Bug Blocks: 221700    
Attachments:
Description Flags
svn(1) diff against the ports tree
none
svn(1) diff against the ports tree none

Description Kyle Evans freebsd_committer 2019-11-27 23:32:51 UTC
Created attachment 209497 [details]
svn(1) diff against the ports tree

As described in great detail elsewhere, Python subprocess will currently loop over every single invalid fd we can give it and attempt to close(2) it. This is very bad for performance, and is absolutely excruciating when trying to debug applications driven by Python.

Python has already stated that they will adopt close_range after Linux merges it, and we will also have an implementation pending in https://reviews.freebsd.org/D21627 for when this happens. We no longer need to come up with an upstreamable solution, as they've chosen the ultimate one and now it's just a waiting game.

In the meantime, everyone on FreeBSD will benefit by applying these patches. They will become naturally invalidated as close_range support trickles in and we can just remove them as it happens.

Default limits will likely be lowered in base soon to help alleviate the problem, but we'll still be executing ~975+ syscalls that don't need to happen (and make truss(1) sad).

Patches based on emaste/cem contributions in PR 221700.

Q/A:
  - Looks good
Comment 1 Kyle Evans freebsd_committer 2019-11-27 23:33:32 UTC
CC'ing python@
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2019-11-29 01:17:01 UTC
Thanks for this Kyle
Comment 3 Kyle Evans freebsd_committer 2019-11-29 02:26:36 UTC
Created attachment 209518 [details]
svn(1) diff against the ports tree

Re-roll all patches, by request of koobs@, to use an #ifdef __FreeBSD__
Comment 4 commit-hook freebsd_committer 2019-11-29 10:56:00 UTC
A commit references this bug:

Author: koobs
Date: Fri Nov 29 10:55:03 UTC 2019
New revision: 518640
URL: https://svnweb.freebsd.org/changeset/ports/518640

Log:
  lang/python{27,35,36,37,38}: Add closefrom(2) support

  A single close(fd) syscall is cheap, but when MAXFDS (maximum file
  descriptor number) is high, the loop calling close(fd) on each file
  descriptor can take several milliseconds.

  The default value of subprocess.Popen "close_fds" parameter changed to True
  in Python 3. Compared to Python 2, close_fds=True can make Popen 10x
  slower: see bpo-37790 [1]

  The present workaround on FreeBSD to improve performance is to load and
  mount the fdescfs kernel module, but this is not enabled by default.

  This change adds minimum viable (and upstreamable) closefrom(2) syscall
  support to Python's subprocess and posix modules, improving performance
  significantly for loads that involve working with many processes, such as
  diffoscope, ansible, and many others.

  For additional optimizations, upstream recently (3.8) landed posix_spawn(2)
  support [3] and has stated that they will adopt close_range(2) after Linux
  merges it [4]. Linux/FreeBSD developers are already collaborating on
  ensuring compatible implementations, with FreeBSD's implementation pending
  in D21627. [5]

  Thank you emaste, cem, kevans for providing analysis, input,
  clarifications, comms/upstream support and patches.

  [1] https://bugs.python.org/issue37790
  [2] https://bugs.python.org/issue38061
  [3] https://bugs.python.org/issue35537
  [4] https://lwn.net/Articles/789023/
  [5] https://reviews.freebsd.org/D21627

  Additional References:

  https://bugs.python.org/issue8052
  https://bugs.python.org/issue11284
  https://bugs.python.org/issue13788
  https://bugs.python.org/issue1663329
  https://www.python.org/dev/peps/pep-0446/

  PR:		242274, 221700
  Submitted by:	kevans (emaste, cem)
  Approved by:	koobs (python (maintainer), santa)

Changes:
  head/lang/python27/Makefile
  head/lang/python27/files/patch-Modules_posixmodule.c
  head/lang/python35/Makefile
  head/lang/python35/files/patch-Modules___posixsubprocess.c
  head/lang/python35/files/patch-Modules_posixmodule.c
  head/lang/python36/Makefile
  head/lang/python36/files/patch-Modules___posixsubprocess.c
  head/lang/python36/files/patch-Modules_posixmodule.c
  head/lang/python37/Makefile
  head/lang/python37/files/patch-Modules___posixsubprocess.c
  head/lang/python37/files/patch-Modules_posixmodule.c
  head/lang/python38/Makefile
  head/lang/python38/files/patch-Modules___posixsubprocess.c
  head/lang/python38/files/patch-Modules_posixmodule.c
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2019-11-29 10:58:02 UTC
Committed, with minor changes (added patch comments), and forgot to mention "MFH: No" in the commit log.

Thank you Kyle for the final patch

I'll proceed to usher this upstream on https://bugs.python.org/issue38061