Bug 232538

Summary: devel/leatherman: fix lengthy closure of open file descriptors
Product: Ports & Packages Reporter: Rick <vrwmiller>
Component: Individual Port(s)Assignee: FreeBSD Puppet Team <puppet>
Status: Closed FIXED    
Severity: Affects Only Me CC: jch, romain
Priority: --- Flags: bugzilla: maintainer-feedback? (puppet)
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch updates closure of open FDs
none
Poudriere build log of devel/leatherman after patch none

Description Rick 2018-10-22 17:24:21 UTC
devel/leatherman built in the default form attempts to close open file descriptors, which can take excessive time when closing large quantities of descriptors. Applying the attached patch mitigates this.

This PR has two attachments:

1) leatherman patch
2) Poudriere build log
Comment 1 Rick 2018-10-22 17:25:05 UTC
Created attachment 198473 [details]
patch updates closure of open FDs
Comment 2 Rick 2018-10-22 17:26:10 UTC
Created attachment 198474 [details]
Poudriere build log of devel/leatherman after patch

This log file illustrates the patched port building successfully.
Comment 3 Romain Tartière freebsd_committer freebsd_triage 2018-10-22 19:20:44 UTC
Hi!

Thanks for the problem report.  Can you please detail what problems this patch is supposed to fix?  Also, can you please fill-in a pull request on GitHub so that the change is accepted upstream, making maintaining the FreeBSD port easier for us?

Thanks!
Comment 4 Rick 2018-10-23 13:57:16 UTC
The copy below summarizes the problem addressed by the patch. Will get a pull request open upstream.

"On some FreeBSD systems sysconf(_SC_OPEN_MAX) returns a very large number, e.g., on one system, it consistently returns 3770208.  The patched code was attempting to close all file descriptors from stderr to sysconf(_SC_OPEN_MAX), and thus ended up calling close() 3770205 times.  This turns out to be quite slow for what it is trying to accomplish.  Replacing this loop with the FreeBSD system call closefrom() accomplishes the same goal, essentially replacing 3.7 million close() system calls with one.

We ran into this issue while running the facter 3.11 binary on a FreeBSD 11.2 system.  All facter invocations ended up taking at least 3 seconds to complete, unless we disabled ruby integration.  We discovered that it was spending most of that 3 seconds calling close() repeatedly."
Comment 5 Romain Tartière freebsd_committer freebsd_triage 2018-10-24 11:28:09 UTC
Neat!

Before:

romain@zappy /tmp % time facter > /dev/null
facter > /dev/null  0,47s user 0,61s system 99% cpu 1,085 total
romain@zappy /tmp % time facter > /dev/null
facter > /dev/null  0,49s user 0,60s system 99% cpu 1,086 total
romain@zappy /tmp % time facter > /dev/null
facter > /dev/null  0,49s user 0,59s system 99% cpu 1,087 total

After:

romain@zappy /tmp % time facter > /dev/null
facter > /dev/null  0,10s user 0,04s system 98% cpu 0,142 total
romain@zappy /tmp % time facter > /dev/null
facter > /dev/null  0,12s user 0,02s system 98% cpu 0,141 total
romain@zappy /tmp % time facter > /dev/null
facter > /dev/null  0,11s user 0,03s system 98% cpu 0,141 total

Thanks for the details, please mention me (@smortex) and link to this issue when you open the PR on GitHub so that I can track this.  closefrom(2) seems to be a BSD/Solaris extension so maybe the final patch will not be as simple as the one you provided, but I guess this patch is a must-have (and a very good starting point).  I will commit it to the ports tree so that we can benefit from this fix while it is being merged upstream.
Comment 6 commit-hook freebsd_committer freebsd_triage 2018-10-24 14:01:57 UTC
A commit references this bug:

Author: romain
Date: Wed Oct 24 11:31:42 UTC 2018
New revision: 482901
URL: https://svnweb.freebsd.org/changeset/ports/482901

Log:
  Fix closure of open file descriptors

  Rely on closefrom(2) instead of a loop on close(2) to close all opened file
  descriptors.  This greatly improve performances.

  While here, commit changes to patches generated by make makepatch.

  With hat:	puppet
  PR:		232538
  Reported by:	 vmiller@verisign.com

Changes:
  head/devel/leatherman/Makefile
  head/devel/leatherman/files/patch-execution_src_posix_execution.cc
  head/devel/leatherman/files/patch-json__container_tests_json__container__test.cc
  head/devel/leatherman/files/patch-locale_src_locale.cc
Comment 7 Romain Tartière freebsd_committer freebsd_triage 2019-01-10 17:53:26 UTC
The fix was committed a few month ago, but I forgot to close the issue at that time.

Closing now, thank you for reporting the problem and submitting this fix!