Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Patch: Rasmus statcallpatch with configure option »

Patch: Rasmus statcallpatch with configure option

php.internals

Derick Rethans

21 years ago
Hei, This is Rasmus' patch including a configure option "--disable-path-normalization" to enable this patch. By default this option is of course turned off. I am planning to commit this patch after 4.3.9 is released too, in case there are no good objections. Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Wez Furlong

21 years ago
There's an #if 0 in there that probably shouldn't be --Wez. On Tue, 14 Sep 2004 14:10:52 +0200 (CEST), Derick Rethans <derick@derickrethans.nl> wrote:

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Wez Furlong wrote:
> There's an #if 0 in there that probably shouldn't be
Yeah, the whole stuff can be removed, as it's added in later again :) I'll create a new patch for it here. Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Derick Rethans wrote:
> On Tue, 14 Sep 2004, Wez Furlong wrote: > > > There's an #if 0 in there that probably shouldn't be > > Yeah, the whole stuff can be removed, as it's added in later again :) > I'll create a new patch for it here.
hmm, no, this was a different patch. No clue why he put that in; Rasmus, can you clarify? Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Wez Furlong

21 years ago
To eliminate an fstat() most likely; it can stay, but it should probably changed to #ifndef SKIP_PATH_CHECKS instead. --Wez. On Tue, 14 Sep 2004 14:50:09 +0200 (CEST), Derick Rethans <derick@php.net> wrote:

Rasmus Lerdorf

21 years ago
On Tue, 14 Sep 2004, Derick Rethans wrote:
> On Tue, 14 Sep 2004, Derick Rethans wrote: > > > On Tue, 14 Sep 2004, Wez Furlong wrote: > > > > > There's an #if 0 in there that probably shouldn't be > > > > Yeah, the whole stuff can be removed, as it's added in later again :) > > I'll create a new patch for it here. > > hmm, no, this was a different patch. No clue why he put that in; Rasmus, > can you clarify?
That should have been a #ifndef SKIP_PATH_CHECKS there. And it is there to get rid of a non-critical ftell(). And yes, there is no doubt that this patch can critically alter the way PHP works and has the potential to break all sorts of stuff. Anything that relies on PHP knowing the full path will break. I had to fix both xdebug and APC, for example. At the same time, enough people have asked me about it that it probably should be more accessible. -Rasmus

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Rasmus Lerdorf wrote:
> That should have been a #ifndef SKIP_PATH_CHECKS there. And it is there > to get rid of a non-critical ftell().
Attached is the patch with this corrected, and corrected white space.
> And yes, there is no doubt that this patch can critically alter the way > PHP works and has the potential to break all sorts of stuff. Anything > that relies on PHP knowing the full path will break. I had to fix both > xdebug and APC, for example. At the same time, enough people have asked > me about it that it probably should be more accessible.
I agree, there are a lot of people who might benefit from it (and do already!) and thus the reason for me to put it into something usable for everybody so that 1) rasmus doesn't have to maintain the patch anymore and 2) users don't have to keep updating their PHP whenever they upgrade PHP. Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Andrey Hristov

21 years ago
Derick Rethans wrote:
> On Tue, 14 Sep 2004, Rasmus Lerdorf wrote: > > >>That should have been a #ifndef SKIP_PATH_CHECKS there. And it is there >>to get rid of a non-critical ftell(). > > > Attached is the patch with this corrected, and corrected white space. > > >>And yes, there is no doubt that this patch can critically alter the way >>PHP works and has the potential to break all sorts of stuff. Anything >>that relies on PHP knowing the full path will break. I had to fix both >>xdebug and APC, for example. At the same time, enough people have asked >>me about it that it probably should be more accessible. > > > I agree, there are a lot of people who might benefit from it (and do > already!) and thus the reason for me to put it into something usable for > everybody so that 1) rasmus doesn't have to maintain the patch anymore > and 2) users don't have to keep updating their PHP whenever they upgrade > PHP. > > Derick >
Both points make sense. Just name the configure option well and put big warnings there that when the option is switched on this _may_ lead to crashing applications. Andrey

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Andrey Hristov wrote:
> Both points make sense. Just name the configure option well and put big warnings there > that when the option is switched on this _may_ lead to crashing applications.
Nothing will crash, it's just that include_once might fail if you're using two different paths to include it. No big deal and easy to fix too IF you need this kind of performance increase. Big warnings is fine with me :) Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Zeev Suraski

21 years ago
At 10:03 15/09/2004, Derick Rethans wrote:
>On Tue, 14 Sep 2004, Andrey Hristov wrote: > > > Both points make sense. Just name the configure option well and put big > warnings there > > that when the option is switched on this _may_ lead to crashing > applications. > >Nothing will crash, it's just that include_once might fail if you're >using two different paths to include it.
I apologize if I didn't make it clear that I did not refer to C level crashes (segfaults) but rather application crashes ('function X redeclared', etc., or apps breaking in a variety of interesting ways - that's a crash all the same). include_once() breaking is a insanely high price to pay that mustn't be taken lightly. It won't be the only thing, anything that relies on opened_path being accurate will not work properly, include_once() is one example, code that uses __FILE__ and php.ini's location in phpinfo() are two others I can think of off hand. There was life before opened_path, and it wasn't pretty - we introduced it for a reason - it was needed in all sorts of places.
>No big deal and easy to fix too >IF you need this kind of performance increase. Big warnings is fine with >me :)
Can you share with us what kind of performance increase (vs. the 'much inferior' realpath cache or even plain vanilla PHP) we're dealing with here? By the way, I can tell you that in real world applications we did not manage to measure any performance gain from the realpath cache, even though we measured 35% performance improvement in a synthetic test, and I expect it to be quite similar with this patch. Before we introduce such a big potential for breakage at the fingertips of every user, let's be sure it's worth it. Again, my view is that it's exactly in the opposite direction of where we've been pushing PHP in the last few years, striving for maximum compatibility even when it comes at a cost. Unless the cost is unbearable (waiting for the numbers) I see no reason to deviate from these guidelines, even if a few people find it interesting. It's the millions who don't that we should think about. Zeev

Rasmus Lerdorf

21 years ago
On Wed, 15 Sep 2004, Zeev Suraski wrote:
> Can you share with us what kind of performance increase (vs. the 'much > inferior' realpath cache or even plain vanilla PHP) we're dealing with > here? By the way, I can tell you that in real world applications we did > not manage to measure any performance gain from the realpath cache, even > though we measured 35% performance improvement in a synthetic test, and I > expect it to be quite similar with this patch. Before we introduce such a > big potential for breakage at the fingertips of every user, let's be sure > it's worth it.
It depends completely on your operating system. Benchmark an application on FreeBSD 4.x that has a lot of includes and you can't miss it. My standard benchmark, which doesn't actually do very many file ops went from 161 requests/second to 195 requests/second with this patch. On operating systems like Linux where syscalls are much lighter, you won't see anywhere near that. But you are still going to see a difference. If you couldn't measure a performance gain then it was lost in some serious other noise in your test. You will typically go from something like 120 stats, seeks and ftells down to 8 or 9 per request with the patch. And by the way, include_once doesn't completely break. It will still catch multiple includes of the same path which is usually what people are interested in. It just won't catch something like: include_once 'file.inc'; include_once '../dir/file.inc'; assuming file.inc is in 'dir'. And while that is significant, in deployments where you have full control over the code or the ability to just whack people and tell them not to do stupid stuff like that, it becomes a non-issue. -Rasmus

Rasmus Lerdorf

21 years ago
The 161->195 looked a bit high to me so I went back through my notes and that is actually for the syscall patch plus the non-pic patch. I don't have numbers for just the syscall patch against stock PHP handy, but I'll see if I can get some time to run some. -Rasmus On Wed, 15 Sep 2004, Rasmus Lerdorf wrote:

Zeev Suraski

21 years ago
At 17:46 15/09/2004, Rasmus Lerdorf wrote:
>On Wed, 15 Sep 2004, Zeev Suraski wrote: > > Can you share with us what kind of performance increase (vs. the 'much > > inferior' realpath cache or even plain vanilla PHP) we're dealing with > > here? By the way, I can tell you that in real world applications we did > > not manage to measure any performance gain from the realpath cache, even > > though we measured 35% performance improvement in a synthetic test, and I > > expect it to be quite similar with this patch. Before we introduce such a > > big potential for breakage at the fingertips of every user, let's be sure > > it's worth it. > >It depends completely on your operating system. Benchmark an application >on FreeBSD 4.x that has a lot of includes and you can't miss it. My >standard benchmark, which doesn't actually do very many file ops went from >161 requests/second to 195 requests/second with this patch.
That's true, I did kind of forget about the other OS's out there, it's very easy to get used to the efficiency of Linux... It would be interesting to see how far the realpath cache can get you with this app. Can you share that standard benchmark of yours?
>On operating systems like Linux where syscalls are much lighter, you won't >see anywhere near that. But you are still going to see a difference. If >you couldn't measure a performance gain then it was lost in some serious >other noise in your test. You will typically go from something like 120 >stats, seeks and ftells down to 8 or 9 per request with the patch.
True, but the bottom line is that it wasn't noticeable in real world apps. It was lost in the noise, except the noise happened to be the application code itself (several apps we took as-is from the web). In a synthetic benchmark designed to measure just the performance of include()'s we measured a significant performance gain (~30%).
>And by the way, include_once doesn't completely break.
[snip] Yes, I would know that. But it breaks when it's most needed. In my experience the circumstances where it won't work properly are extremely common.
>assuming file.inc is in 'dir'. And while that is significant, in >deployments where you have full control over the code or the ability to >just whack people and tell them not to do stupid stuff like that, it >becomes a non-issue.
I don't see why you think this is 'stupid' (of course, when it's not done within the same file). Again, it's extremely common and actually the whole point behind include_once() . I do agree that if you have centralized control over your code you can force people not to use it, but that can apply to just about any other feature in PHP. We can probably have some --disable-indirect-reference switch that might enable us to get an extra bit of performance, do we want to get into this business? I think not. Where do we draw the line? The only way to draw it is to make it stupid-simple - if it allows users to selectively break (or alter) language functionality, it shouldn't be in the standard distribution. We have some old leftovers that don't follow these guidelines, but at least most of them are changeable in runtime (allowing for portable code), and all of them are at the very least can be configured in php.ini. I *think* you agree with that, and that we're best off putting it in a special location, maybe on php.net (if you disagree and I'm putting words in your mouth, my apologies and I take it back). Zeev

Derick Rethans

21 years ago
On Wed, 15 Sep 2004, Zeev Suraski wrote:
> That's true, I did kind of forget about the other OS's out there, it's very > easy to get used to the efficiency of Linux... > It would be interesting to see how far the realpath cache can get you with > this app. Can you share that standard benchmark of yours?
We have "instructed" a customer to test it (running freebsd) and he'll benchmark it, and also watch diskio and sustem load. When they report back I'll share it of course.
> Yes, I would know that. But it breaks when it's most needed. In my > experience the circumstances where it won't work properly are extremely common.
But for those people this patch isn't interesting anyway. It's only interesting if you have a dedicated server running. People can easily test their applications for this too.
> I *think* you agree with that, and that we're best off putting it in a > special location, maybe on php.net (if you disagree and I'm putting words > in your mouth, my apologies and I take it back).
Patches are still a hassle to maintain, especially if you're mixing two of them running it on the same code. THey also need updating often due to changes in the distribution. Thinks like that are annoying, although I don't see a better solution for this now (except by doing it the kernel way and use --enable-expirental-foo for example). Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Rasmus Lerdorf

21 years ago
On Wed, 15 Sep 2004, Zeev Suraski wrote:
> That's true, I did kind of forget about the other OS's out there, it's very > easy to get used to the efficiency of Linux... > It would be interesting to see how far the realpath cache can get you with > this app. Can you share that standard benchmark of yours?
http://lerdorf.com/bench.tar It's actually a really crappy benchmark. When you profile it you see that it spends most of its time in var_dump(). I have been working on a better one that touches more of PHP evenly.
> I *think* you agree with that, and that we're best off putting it in a > special location, maybe on php.net (if you disagree and I'm putting words > in your mouth, my apologies and I take it back).
I am not sure what the right solution is. I would like to see this patch and others like it be more visible in a way that when someone changes something that either affects the patch or perhaps even incorporates part of it, the patch is updated. I have a number of these sitting around that could be very useful but are somewhat limited in terms of their audience. We need some sort of better balance between safe vanilla code that runs everywhere for everyone and platform-specific or audience-specific stuff. Derick and I are both deploying controlled code on a huge scale and turning off edge-case stuff we know we will never use can make a big difference. Even a 1 or 2 req/sec difference is significant when you serve up billions of pages a day. A good example of that is the pipe checks. I really don't need the ability to pipe scripts to be parsed by PHP and I am not willing to take the 2-syscall hit on every one of my requests to support this. Or the non-pic patch which we can't really deploy on a large scale because libtool is too stupid, but this patch provides an instant 10-20% performance increase on both ia32 FreeBSD and Linux while breaking ia64 Linux and AIX I think. Having the patches in the code itself could lead to an #if mess, especially since they may conflict. But on the other hand having them there would get people thinking about whether some of these things could be worked around or done in a better way and code changes would automatically be reflected in them. Perhaps a decent patch repository is the way to go. One that would keep track of patches by PHP version number and branches and periodically test-apply the patches against the trees and notify the patch maintainers of breakage. I don't really care about this particular patch, and I am nowhere near as keen as Derick on seeing a configure switch for it, what I would like to see is a system for dealing with these patches that would encourage more of them which in turn would lead to a lot of new ideas flowing into the project. -Rasmus

Andi Gutmans

21 years ago
Can someone who has more configure knowledge than me volunteer to make our build system use non-pic for Linux/FreeBSD ia32? Andi

Zeev Suraski

21 years ago
At 19:29 14/09/2004, Derick Rethans wrote:
> > And yes, there is no doubt that this patch can critically alter the way > > PHP works and has the potential to break all sorts of stuff. Anything > > that relies on PHP knowing the full path will break. I had to fix both > > xdebug and APC, for example. At the same time, enough people have asked > > me about it that it probably should be more accessible. > >I agree, there are a lot of people who might benefit from it (and do >already!) and thus the reason for me to put it into something usable for >everybody so that 1) rasmus doesn't have to maintain the patch anymore >and 2) users don't have to keep updating their PHP whenever they upgrade >PHP.
I don't disagree that it can be useful. But such a feature should not come bundled as an option in PHP. The Right(tm) solution here is to provide the realpath cache, which will speed performance up without breaking compatibility, and hopefully we'll have that one real soon now. We can maybe look into backporting it to PHP 4 as an optional feature (just to make sure we don't break apps in case it's buggy). Adding this patch as an option goes against everything that we've been trying to do in the last few years. This useful patch should be made available so that those who really need it can find it, but it should come up with a huge disclaimer, and it should not be trivial for users to enable it in the form of --enable-foo. I see the fact that people have to invest some work in using it as a good thing, since if a lot of people start using it, it can become a very big headache very quickly. Zeev

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Zeev Suraski wrote:
> I don't disagree that it can be useful. But such a feature should not come > bundled as an option in PHP. The Right(tm) solution here is to provide the > realpath cache, which will speed performance up without breaking > compatibility, and hopefully we'll have that one real soon now. We can > maybe look into backporting it to PHP 4 as an optional feature (just to > make sure we don't break apps in case it's buggy).
The realpath cache is not nearly what Rasmus patch does. Besides that, the patch seems to be rock solid (we've customers running it in production now).
> Adding this patch as an option goes against everything that we've been > trying to do in the last few years. This useful patch should be made > available so that those who really need it can find it, but it should come > up with a huge disclaimer, and it should not be trivial for users to enable > it in the form of --enable-foo. I see the fact that people have to invest > some work in using it as a good thing, since if a lot of people start using > it, it can become a very big headache very quickly.
--enable-foo=40digit-prime-of-your-choice? For people who use it, they would use this professionally. They are most often not even *allowed* to patch their PHP nor do they want to spend time on applying this patch each time. Also, it's not an inherently bad patch either. You just need to make sure that 1) you don't use symlinks and 2) always include a file with the same file path. Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Zeev Suraski

21 years ago
I don't think we should have this patch in the distribution. It shouldn't be that accessible as it has the potential to break behavior of both apps and plugins. I think it belongs in a patches repository such as http://www.zend.com/zend/week/pat/ (doesn't have to be that one in particular, but something like that). (There are also several coding style issues with the patch if we do decide to accept it) Zeev At 15:10 14/09/2004, Derick Rethans wrote:

Andi Gutmans

21 years ago
Hey Derick and all, As I mentioned in the past, I think solving it in a completely compatible manner is the best way to go. I sent out the realpath() cache patch which addresses the most problematic file system issues (and if there are any other things we could probably address too). I think there's a big advantage in having something which is completely compatible because we could enable it by default and all PHP users would benefit from it. I volunteer to put some gas on getting this patch production ready. Maybe I can send a new version in a couple of days and discuss what else it is missing? Andi At 02:10 PM 9/14/2004 +0200, Derick Rethans wrote:

Rasmus Lerdorf

21 years ago
The patch doesn't just address realpath though. We have a number of other redundant/non-essential syscalls in the streams code, for example. And the realpath cache is still heavier than not worrying about the full path at all. I do agree that however people get this patch, they need to understand that they are not running standard PHP and it is likely to break many things. At the same time, having it in CVS and maintainable by others and having a single known version of this would help the people who use it and the people who want to tweak their extensions to support it. Perhaps we need a central php.net hosted and CVS-backed place for unsupported patches and nasty hacks like this? Useful ideas that are applicable to a wider audience do often grow out of some of these crazy hacks. -Rasmus On Tue, 14 Sep 2004, Andi Gutmans wrote:

Andi Gutmans

21 years ago
At 10:52 AM 9/14/2004 -0700, Rasmus Lerdorf wrote:
>The patch doesn't just address realpath though. We have a number of other >redundant/non-essential syscalls in the streams code, for example. And >the realpath cache is still heavier than not worrying about the full path >at all.
Can you describe the other non-essential syscalls? I want to be sure we really can't cover those (probably some of them we can't). As far as the realpath patch goes (which is probably the biggest problem), I think that if we have the realpath cache in CVS that part of your patch won't be needed because I think the additional few K of memory wouldn't make it any noticeably slower even on loaded systems. So let me put some gas on that patch and that would allow to at least cut down your patch a bit.
>I do agree that however people get this patch, they need to understand >that they are not running standard PHP and it is likely to break many >things. At the same time, having it in CVS and maintainable by others and >having a single known version of this would help the people who use it and >the people who want to tweak their extensions to support it. > >Perhaps we need a central php.net hosted and CVS-backed place for >unsupported patches and nasty hacks like this? Useful ideas that are >applicable to a wider audience do often grow out of some of these crazy >hacks.
Yes, I definitely think that is needed because there have been some patches floating around which probably shouldn't be in the core distribution but should be out there with big disclaimers for people who want to use them. I know Steph was working on something like that because she also felt there needs to be easier access to such patches (and I agree). I'm not sure where that project is at right now. Andi

Rasmus Lerdorf

21 years ago
On Tue, 14 Sep 2004, Andi Gutmans wrote:
> Can you describe the other non-essential syscalls? I want to be sure we > really can't cover those (probably some of them we can't). > As far as the realpath patch goes (which is probably the biggest problem), > I think that if we have the realpath cache in CVS that part of your patch > won't be needed because I think the additional few K of memory wouldn't > make it any noticeably slower even on loaded systems. So let me put some > gas on that patch and that would allow to at least cut down your patch a bit.
Well, you can mostly just read the part of the patch that addresses streams.c. I skip the check to see if we are dealing with a pipe in the _php_stream_fopen_from_file() call. This change trickles down and removes a number of small bits that check that is_pipe flag. And I also don't care what position the file pointer is at, so I skip the ftell() just after that which also eliminates a seek in _php_stream_cast(). And in _php_stream_fopen() there is an include/require sanity check fstat() that I also have no use for. It's mostly just looking at the common path and watching the syscalls and getting rid of anything that isn't logically needed to serve up a normal PHP page. I understand that some of these checks are necessary for edge-case things, but when an edge-case check slows down the common case I sacrifize that edge. -Rasmus

Andi Gutmans

21 years ago
At 11:30 AM 9/14/2004 -0700, Rasmus Lerdorf wrote:
>On Tue, 14 Sep 2004, Andi Gutmans wrote: > > Can you describe the other non-essential syscalls? I want to be sure we > > really can't cover those (probably some of them we can't). > > As far as the realpath patch goes (which is probably the biggest problem), > > I think that if we have the realpath cache in CVS that part of your patch > > won't be needed because I think the additional few K of memory wouldn't > > make it any noticeably slower even on loaded systems. So let me put some > > gas on that patch and that would allow to at least cut down your patch > a bit. > >Well, you can mostly just read the part of the patch that addresses >streams.c. I skip the check to see if we are dealing with a pipe in the >_php_stream_fopen_from_file() call. This change trickles down and removes >a number of small bits that check that is_pipe flag. And I also don't >care what position the file pointer is at, so I skip the ftell() just >after that which also eliminates a seek in _php_stream_cast(). > >And in _php_stream_fopen() there is an include/require sanity check >fstat() that I also have no use for. > >It's mostly just looking at the common path and watching the syscalls and >getting rid of anything that isn't logically needed to serve up a normal >PHP page. I understand that some of these checks are necessary for >edge-case things, but when an edge-case check slows down the common case I >sacrifize that edge.
Yep it seems like some of these are things which a realpath() cache patch would not address. As I mentioned earlier we should make your patch available but I think it's a bad idea to have it in the default distro. I suggest the following: a) I will try and send internals@ an updated version of the realpath() cache in the next few days. This should give a lot of bang for the buck because realpath() is probably the suckiest system call in the startup. b) Maybe Wez & Sara can take one more look to double check if there aren't any checks they can possibly save without impairing functionality. c) Create a new version of your patch based on (a) & (b) and make sure we find an accessible place for it with the disclaimer. Maybe Steph can take care of that. Thanks, Andi

Rasmus Lerdorf

21 years ago
On Tue, 14 Sep 2004, Andi Gutmans wrote:
> a) I will try and send internals@ an updated version of the realpath() > cache in the next few days. This should give a lot of bang for the buck > because realpath() is probably the suckiest system call in the startup.
Make sure you update it to use the new sapi_get_request_time() call to get the time on each request to avoid a gettimeofday() syscall. -Rasmus

Andi Gutmans

21 years ago
Yeah I'll try and see what can be done. It might be a bit tricky because TSRM doesn't "know" PHP but I think it can be solved. Andi At 04:33 PM 9/14/2004 -0700, Rasmus Lerdorf wrote:

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Andi Gutmans wrote:
> a) I will try and send internals@ an updated version of the realpath() > cache in the next few days. This should give a lot of bang for the buck > because realpath() is probably the suckiest system call in the startup. > b) Maybe Wez & Sara can take one more look to double check if there aren't > any checks they can possibly save without impairing functionality. > c) Create a new version of your patch based on (a) & (b) and make sure we > find an accessible place for it with the disclaimer.
A good accessible place would be our distribution. It's annoying to have to maintain a patch outside the main tree. There is also no reason why people would just enable this feature by default if they have no clue what they are doing, we can add big nice disclaimers around it. I can see nothing wrong with it, I also don't think this is a nasty hack or a crappy patch. I do not seek to have this patch into PHP 5, you can do your realcache magic there if you want. Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Jan Schneider

21 years ago
Zitat von Derick Rethans <derick@php.net>:
> On Tue, 14 Sep 2004, Andi Gutmans wrote: > >> a) I will try and send internals@ an updated version of the realpath() >> cache in the next few days. This should give a lot of bang for the buck >> because realpath() is probably the suckiest system call in the startup. >> b) Maybe Wez & Sara can take one more look to double check if there aren't >> any checks they can possibly save without impairing functionality. >> c) Create a new version of your patch based on (a) & (b) and make sure we >> find an accessible place for it with the disclaimer. > > A good accessible place would be our distribution. It's annoying to have > to maintain a patch outside the main tree. There is also no reason why > people would just enable this feature by default if they have no clue > what they are doing, we can add big nice disclaimers around it. I can > see nothing wrong with it, I also don't think this is a nasty hack or a > crappy patch. I do not seek to have this patch into PHP 5, you can do > your realcache magic there if you want.
Why not keeping that patch (and others that might be worth it as well) in the php-src module, but really as a patch, not applied to the default distro? This way it's available at a single point, everyone having a tarball can apply it, and it still won't be available with a simple config option. Jan.
-- Do you need professional PHP or Horde consulting? http://horde.org/consulting/

Derick Rethans

21 years ago
On Wed, 15 Sep 2004, Jan Schneider wrote:
> Why not keeping that patch (and others that might be worth it as well) in > the php-src module, but really as a patch, not applied to the default > distro? This way it's available at a single point, everyone having a > tarball can apply it, and it still won't be available with a simple config > option.
Because it won't solve the main thing: maintenance. And patching is not something that a lot of people are familiar with, it's also a hassle to do. Derick

Adam Maccabee Trachtenberg

21 years ago
On Wed, 15 Sep 2004, Derick Rethans wrote:
> There is also no reason why people would just enable this feature by > default if they have no clue what they are doing, we can add big > nice disclaimers around it.
We see idiots submit bug reports with all sorts of configure options they should never use, like --enable-experimental-zts, which is described in configure as "This will most likely break your build". -adam
-- adam@trachtenberg.com author of o'reilly's "upgrading to php 5" and "php cookbook" avoid the holiday rush, buy your copies today!

Derick Rethans

21 years ago
On Wed, 15 Sep 2004, Adam Maccabee Trachtenberg wrote:
> On Wed, 15 Sep 2004, Derick Rethans wrote: > > > There is also no reason why people would just enable this feature by > > default if they have no clue what they are doing, we can add big > > nice disclaimers around it. > > We see idiots submit bug reports with all sorts of configure options > they should never use, like --enable-experimental-zts, which is > described in configure as "This will most likely break your build".
That's why we have quickfixes in our bug system. ALthough I can not remember somebody who used this setting and filed a bug because of it. Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Bård Farstad

21 years ago
> >It's mostly just looking at the common path and watching the syscalls and > >getting rid of anything that isn't logically needed to serve up a normal > >PHP page. I understand that some of these checks are necessary for > >edge-case things, but when an edge-case check slows down the common case I > >sacrifize that edge. > > Yep it seems like some of these are things which a realpath() cache patch > would not address. As I mentioned earlier we should make your patch > available but I think it's a bad idea to have it in the default distro. > I suggest the following: > a) I will try and send internals@ an updated version of the realpath() > cache in the next few days. This should give a lot of bang for the buck > because realpath() is probably the suckiest system call in the startup. > b) Maybe Wez & Sara can take one more look to double check if there aren't > any checks they can possibly save without impairing functionality. > c) Create a new version of your patch based on (a) & (b) and make sure we > find an accessible place for it with the disclaimer. Maybe Steph can take > care of that.
As I'm very interested in this patch and patches like this I send this reply to make a suggestion. 1) This patch can and will break some applications, the users needs to be aware of this. So a big warning needs to be shown to the user. 2) Application vendors should be able to advice their customers to tune PHP to run an application and the vendor will then say which patch you can safely run or not. So the patch should be available easily. 3) Having patches available means that they can be improved by many people and Rasmus doesn't need to maintain this, which means less work load on him. 4) Making the realpath() cache is a good thing, but it is not the same. So like winnie the pooh I would say yes, both please. 5) If we create a patch repository somewhere this would make the patches available, but it will be much more hazzle for the users - which is not good. If a user wants to use several patches from this repository he needs to download the patches verify that he has the correct version and also verify that one patch does not break another. The latter point can quickly be an issue since if you have a patch repository then it could be a pain to make sure that all patches are compatible with eachother. What about doing it similar to the way the Linux kernel does this. We could add --experimental compile options. E.g. --experimental- (or --non-standard- or another good indicating name for that matter ). So adding --exeprimental-minimize-statcall could enable this patch. After the build is done there should be a big fat warning about the different experimental patches with links to information about them. E.g. detailed information about what they will break. In phpinfo() it should also be a list with experimental options, mabye even in bold/red font to make it very clear. Atleast I find it simpler to advice e.g. a hosting company to enable --experimental--foo to boost performance rather than tell them to download, make sure you have the correct version and patch. And in the end it's the end users we would like to make things simpler for right? Atleast this is what I think about the matter. We would gladly spend the time eneded to make what I describe above. Cheers,
-- Bård Farstad bf@ez.no | eZ systems | http://ez.no

Derick Rethans

21 years ago
On Tue, 14 Sep 2004, Andi Gutmans wrote:
> At 10:52 AM 9/14/2004 -0700, Rasmus Lerdorf wrote: > >The patch doesn't just address realpath though. We have a number of other > >redundant/non-essential syscalls in the streams code, for example. And > >the realpath cache is still heavier than not worrying about the full path > >at all. > > Can you describe the other non-essential syscalls? I want to be sure we > really can't cover those (probably some of them we can't). > As far as the realpath patch goes (which is probably the biggest problem), > I think that if we have the realpath cache in CVS that part of your patch
Will you backport that realpath cache to PHP 4.3? Derick
-- Derick Rethans http://derickrethans.nl | http://ez.no | http://xdebug.org

Andi Gutmans

21 years ago
At 09:05 AM 9/15/2004 +0200, Derick Rethans wrote:
>On Tue, 14 Sep 2004, Andi Gutmans wrote: > > > At 10:52 AM 9/14/2004 -0700, Rasmus Lerdorf wrote: > > >The patch doesn't just address realpath though. We have a number of other > > >redundant/non-essential syscalls in the streams code, for example. And > > >the realpath cache is still heavier than not worrying about the full path > > >at all. > > > > Can you describe the other non-essential syscalls? I want to be sure we > > really can't cover those (probably some of them we can't). > > As far as the realpath patch goes (which is probably the biggest problem), > > I think that if we have the realpath cache in CVS that part of your patch > >Will you backport that realpath cache to PHP 4.3?
I don't see a reason why not to backport this patch to 4.3. Andi