Bug 242274 - lang/python{27,35,36,37,38}: Add closefrom(2) support
Summary: lang/python{27,35,36,37,38}: Add closefrom(2) support
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Kubilay Kocak
URL:
Keywords:
Depends on:
Blocks: 221700
  Show dependency treegraph
 
Reported: 2019-11-27 23:32 UTC by Kyle Evans
Modified: 2019-11-29 10:58 UTC (History)
2 users (show)

See Also:
koobs: merge-quarterly-


Attachments
svn(1) diff against the ports tree (12.75 KB, patch)
2019-11-27 23:32 UTC, Kyle Evans
no flags Details | Diff
svn(1) diff against the ports tree (13.09 KB, patch)
2019-11-29 02:26 UTC, Kyle Evans
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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