Project

General

Profile

Bug #10206

Optimize pipesubr.pipeopen()

Added by Anonymous over 5 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Important
Assignee:
William Grzybowski
Category:
Middleware
Target version:
Severity:
New
Reason for Closing:
Reason for Blocked:
Needs QA:
Yes
Needs Doc:
Yes
Needs Merging:
Yes
Needs Automation:
No
Support Suite Ticket:
n/a
Hardware Configuration:
ChangeLog Required:
No

Description

Currently pipeopen() utilizes subprocess.Popen() with close_fds=True. The Python 2.7 implementation of close_fds is pretty horrible and essentially does a loop from 3 to kern.maxfiles and attempts to close each file descriptor one at a time. On a system I have with 96GB of RAM this translates to a loop that iterates over 3 million times and causes any pipeopen() call to take 0.4s just to begin to launch. I'm actually amazed it's as fast as it is considering what is being done!

What I've described above has a pretty serious usability impact if you have jails configured to not use DHCP because it causes repeated sipcalc's to be exec'ed (see JailsConfiguration.__init__). I don't know if that behavior is intentional or not but with just 4 jails it creates over two dozen sipcalc calls which make it take over 10 seconds just to render the left navigation bar.

There are several ways this could be addressed but one that doesn't seem too disruptive is mounting fdescfs:

mount -t fdescfs null /dev/fd

And then apply the attached patch. I have not thoroughly tested this but it seems to work fine and the UI goes from being basically unusably slow to incredibly snappy.

Anyway, I mainly did this to scratch an itch but thought I'd toss it in here to shed some light on an underlying issue and maybe this is a fix you'd be interested in.

close_fds.patch (1.09 KB) close_fds.patch Anonymous, 06/15/2015 12:42 PM

Associated revisions

Revision f000ada4 (diff)
Added by William Grzybowski over 5 years ago

Close fds on our own as default implementation is very suboptimal Ticket: #10206 Merge-FN93: yes Merge-TN93: yes

Revision 7bc42337 (diff)
Added by William Grzybowski over 5 years ago

Close fds on our own as default implementation is very suboptimal Ticket: #10206 Merge-FN93: yes Merge-TN93: yes (cherry picked from commit f000ada4e0a69215642ee7cae88cb28431214503)

Revision 7533ac5d (diff)
Added by William Grzybowski over 5 years ago

Close fds on our own as default implementation is very suboptimal Ticket: #10206 Merge-FN93: yes Merge-TN93: yes (cherry picked from commit f000ada4e0a69215642ee7cae88cb28431214503)

History

#1 Updated by Jordan Hubbard over 5 years ago

  • Category changed from 148 to 3
  • Assignee changed from Anonymous to William Grzybowski
  • Priority changed from No priority to Important
  • Target version set to Unspecified

Reassigning to the right component. Sometimes, I wonder if FreeNAS 10 shouldn't have its own entirely separate bug tracker. That would cause its own problems of course, but...

#2 Updated by William Grzybowski over 5 years ago

  • Status changed from Unscreened to Screened

#3 Updated by William Grzybowski over 5 years ago

I think we could use fstat -p or simply iterate over a small number, like 1024, since we dont expect the GUI (which is the process spawning the processes) to ever have that many open file descriptors.

#4 Updated by Anonymous over 5 years ago

FWIW, the fixed lower number seems like a reasonable fix to me.

Very rough benchmarks for my system (E5-2620v3 CPU/96GB RAM which yields kern.maxfiles = 3141498) giving the lowest # for 10 runs are -

As it is today w/ close_fds=True: 405ms delay
Proposed patch: 2.6ms with 4 extra fd's open and of course gets worse depending on # actually open
Fixed at 1024: 4.3ms

Even fixed at 1K it's roughly a 100x improvement.

#5 Updated by William Grzybowski over 5 years ago

  • Status changed from Screened to Ready For Release

#6 Updated by Jordan Hubbard over 5 years ago

  • Status changed from Ready For Release to Resolved

#7 Avatar?id=14398&size=24x24 Updated by Kris Moore about 4 years ago

  • Target version changed from Unspecified to N/A

Also available in: Atom PDF