php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #56233 [suggestion] key compare improvement
Submitted: 2004-11-17 07:48 UTC Modified: 2006-02-16 18:34 UTC
From: xuefer at 21cn dot com Assigned:
Status: Closed Package: APC (PECL)
PHP Version: Irrelevant OS: all
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: xuefer at 21cn dot com
New email:
PHP Version: OS:

 

 [2004-11-17 07:48 UTC] xuefer at 21cn dot com
Description:
------------
1. use != instead of < for mtime compare
2. use atime if ctime > mtime
was done before but reverted, Smarty and other compile language need this, without it cause it hard to update Smarty Modules.(must restart apache)
comments from source:
    /*
     * I generally disagree with using the ctime here because you lose the 
     * ability to warm up new content by saving it to a temporary file, hitting
     * it once to cache it and then renaming it into its permanent location.
     */
i doubt if "warm up" really help if the content is generate on the fly.
suppose there're many processes check for a file, realizing it not exists
if (file_need_update($file)) {
  mylock(); <--- all other locked here
  if (file_need_update($file)) { // recheck
    generate($file); <--- only 1 get here
    hit_the_file_by_include(); // <- to hit or not to hit, this is a question
    rename();
  }
  myunlock();
}
include($file);

all other process is locked there! why do we need hit before rename?
if we don't hit, just rename, everything is same, all process locked on include($file), recompile and reoptimize
this is problem of apc, not phpscript.
apc should re-check the file and cache entry AFTER lock and BEFORE compile, this means only 1 process can compile a file(not on "each" file), this is much better than many process compile 1 file.
well, this should be another suggestion, not ctime problem

3. add filesize compare, not only inode/device
many ftp software, including scp, when uploading file in a heavy site, will cause apc cached half of a file.



Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2004-11-18 10:21 UTC] rasmus@php.net
1. The comparison is:
   if(t - buf.st_mtime < 2)
   Are you suggesting changing this to != ?
   That doesn't make any sense.

2. We don't lock until we try to insert the new cache entry.
   That means we have to allocate a number of things before the lock.  By warming up new entries by hitting them once before making them live we avoid this rush of multiple processes all trying to allocate the memory needed to do an apc_cache_insert().  And by the way, the Smarty folks realized it was stupid to backdate things like that and they no longer do it so Smarty doesn't need this.

3. Yes, a filesize check makes sense.  I looked at adding this before.
 [2004-11-18 10:44 UTC] xuefer at 21cn dot com
1.
i'm lost

i see only:
            if ((*slot)->key.mtime < key.mtime) {
where is < 2 ?

2. by read the source, i know we don't lock until cache_insert, but how about adding 1 more lock?
in my previous example, there's already 2 locks: 1 my*lock(), 2 include(), the one by apc
lock(compile_lock) (different from cache lock)
recheck();
allocate()
complile()
optimize()
apc_cache_insert() (which also do lock/unlock)
unlock(compile_lock)

this, however, make only 1 file (and by 1 process) at a time.
but: 1. it's good for "after restart", or a "cache clear". also simpler than user do themself
2. user can do nothing with static php file, apc should take care of warming up.
3. win32 have no inode, only filename(if we implement it), rename()+hit is useless to warm up
although, win32 is only a devel-test for me
 [2004-11-18 10:55 UTC] rasmus@php.net
1. http://cvs.php.net/co.php/pecl/apc/apc_cache.c?r=3.74
Line 550

2. Locks are expensive.  The less of them the better.

3. I really don't care about win32 and I certainly don't want a win32 problem affecting non-win32 platforms.  #ifdef a different approach for win32.
 [2004-11-18 11:00 UTC] rasmus@php.net
And by the way, using the filename as a key is way too expensive.  I don't know the filename in my version of PHP, for example because I removed the expensive realpath() call so if you do include "./foo.php" in one directory and then do include "./foo.php" in another directory these two ./foo.php files are obviously different but without a realpath() call they look the same.  A stat to get the device+inode is much much quicker than calling realpath() on it.
 [2004-11-18 11:15 UTC] xuefer at 21cn dot com
oops, i didn't see the cvs before i post this bug
i never meant to hurt "if(t - buf.st_mtime < 2)"

1. the request for != is about "if ((*slot)->key.mtime < key.mtime) {".
my point is: some ftp may preserve mtime when uploading or downloading. well, maybe i (and other users) should configure the client not to do so. but i still don't know why we need < not !=

2. lock is expensive, but not each request need compile_lock. only when we're going to compile.
if (check_if_should_compile()) {
 lock(compile_lock);
 if (check_if_should_compile()) { // again
    ......
 }
 unlock(compile_lock);
}

your slam_defense way looks good for me. it won't make too much child locking together even without a nonblocking-lock

3. i never meant to remove inode checking, and yes, use #ifdef
(filename for win32) + (inode for most other system) is done by mmcache. apc can do it too.
 [2004-11-24 05:41 UTC] rasmus@php.net
I think you are right about

  if ((*slot)->key.mtime < key.mtime)

I can't think of a good reason for not making that a != check.
George, Dan, anybody else?
 [2006-02-16 18:34 UTC] ilia at prohost dot org
This bug has been fixed in CVS.

In case this was a documentation problem, the fix will show up at the
end of next Sunday (CET) on pecl.php.net.

In case this was a pecl.php.net website problem, the change will show
up on the website in short time.
 
Thank you for the report, and for helping us make PECL better.

This bug has been fixed in CVS.

In case this was a documentation problem, the fix will show up at the
end of next Sunday (CET) on pecl.php.net.

In case this was a pecl.php.net website problem, the change will show
up on the website in short time.
 
Thank you for the report, and for helping us make PECL better.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Mar 28 15:01:29 2024 UTC