Opened 3 years ago

Closed 3 years ago

#1487849 closed Feature Patches (fixed)

File system performance patch

Reported by: vminakov Owned by:
Priority: 5 Milestone: 0.6-beta
Component: Core functionality Version: 0.4.2
Severity: normal Keywords:
Cc:

Description

Hello guys,

just wanted to share some thoughts on file system optimization. The problem arose, when we deployed roundcube on our production platform. Load average on 2 servers (24 cores, 16GB ram) went to 70. And that's with nginx, php-fpm and apc turned on.

The bottleneck was file system. Roundcube was running on NFS (I know, that's absolutely ridiculous) and was doing almost a million 'stat' calls per minute. The reason for that is include path.

$include_path = INSTALL_PATH . PATH_SEPARATOR;
$include_path.= INSTALL_PATH . 'program' . PATH_SEPARATOR;
$include_path.= INSTALL_PATH . 'program/lib' . PATH_SEPARATOR;
$include_path.= INSTALL_PATH . 'program/include' . PATH_SEPARATOR;

So, whenever you need a class like rcube_session, PHP makes 4 stat calls (on '.', on 'program/', on 'program/lib' and on 'program/include'), which are very expensive not only in NFS, but in other file systems as well. In order to get rid of useless stat calls, I reduced the include path, tried to move everything that's possible to autoloader, and where it wasn't possible, used absolute paths. Load dropped to 1.5.

Autoloader allows you to optimize class loading in one place, without the need to refactor everything. Unfortunately, Roundcube doesn't rely heavily on autoloader. Probably, because it lacks clear folder structure. Library classes mixed with function files and core classes, no naming conventions and no hierarchical folder structure. Look at Zend Framework. Although it's being criticised for lack of performance, it has a very clear structure and requires just one include path: autoloader knows exactly where class files are.

Absolute paths also have a benefit, which is often left unnoticed. They allow you to turn off 'stat' in APC. That is, if the file has been cached by APC, PHP will never do 'stat' calls to check, if the file has been modified. And it's only possible with absolute paths http://www.php.net/manual/en/apc.configuration.php#ini.apc.stat. Turning off 'stat' in APC led to another drop in load averages: down to 0.7.

I've attached a patch, which is dirty, but gives an idea what I've done. I really think that more attention should be given to refactoring. And not just for a sake of it.

Attachments (1)

changeset_r314_r306.diff (8.3 KB) - added by vminakov 3 years ago.

Download all attachments as: .zip

Change History (6)

Changed 3 years ago by vminakov

comment:1 Changed 3 years ago by vminakov

  • Component changed from Addressbook to Core functionality
  • Type changed from Bugs to Feature Patches
  • Version changed from 0.5.1 to 0.4.2

comment:2 Changed 3 years ago by alec

  • Milestone changed from later to 0.6-beta

You could test with current svn-trunk code, rcube_autoload() function has been changed. Next, we'll not change code of PEAR's packages. The rest of the patch (including of roundcube files) looks ok.

comment:3 Changed 3 years ago by vminakov

Yes, I saw the autoloader version in trunk and it doesn't address the issue I was talking about. It requires the same include path, the same amount of stat calls.

If you don't want to change code of PEAR packages, then why do keep them in repository and not as external pear dependencies? Most of the PEAR packages are licensed under BSD, MIT, GPL or PHP licenses, which allow modification and redistribution. And as I have said earlier, using vendor branches to keep track of you own modifications is very easy.

The rest of the patch won't make a large impact.

comment:4 Changed 3 years ago by alec

Partially applied in [4351f7cd]. We're providing Roundcube optional package without external libraries like PEAR. So, users can use libraries installed in their system/distribution. I leave this ticket open further decission. Maybe we could provide optional iniset.php file or detect once the existence of e.g. lib/PEAR.php or sth.

comment:5 Changed 3 years ago by alec

  • Resolution set to fixed
  • Status changed from new to closed

More improvements in [926948e7].

Note: See TracTickets for help on using tickets.