Hi!
I've implemented a patch that allows to "mask" certain types of errors -
i.e. make PHP engine completely ignore them. Now even if the error is
not reported, it passes full cycle through message string creation, all
allocations on the way, etc. even though ultimately the result of it
will be thrown out. So I offer a way to make unwanted errors just disappear.
The patch uses new directive - "error_mask", and reuses
orig_error_reporting global (which is not used anywhere in PHP source so
I wonder why it exists at all, but that's certainly convenient :) which
makes it binary-compatible and fit for 5.3. Of course, in HEAD we could
rename it.
If the value in not 0, that's the mask which would filter out all errors
that do not fit the mask (i.e. you could say in production that
everything but warnings and errors should be discarded), and if the
value is 0 then the mask is the same as error_reporting - i.e. it can be
more dynamic and account for @, etc. and all non-reported errors will be
discarded. Fatal errors will never be discarded.
Alternative approach may be to just discard all non-reported errors, but
that could be a problem with user error handlers and extension-supplied
error callbacks, so the proposed approach is more flexible as you could
control discarding and reporting separately.
So, what do you think?
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Stas,
While the patch is certainly interesting, I think masking errors is a
really really bad idea.
Hi!
I've implemented a patch that allows to "mask" certain types of
errors - i.e. make PHP engine completely ignore them. Now even if
the error is not reported, it passes full cycle through message
string creation, all allocations on the way, etc. even though
ultimately the result of it will be thrown out. So I offer a way to
make unwanted errors just disappear.
The patch uses new directive - "error_mask", and reuses
orig_error_reporting global (which is not used anywhere in PHP
source so I wonder why it exists at all, but that's certainly
convenient :) which makes it binary-compatible and fit for 5.3. Of
course, in HEAD we could rename it.
If the value in not 0, that's the mask which would filter out all
errors that do not fit the mask (i.e. you could say in production
that everything but warnings and errors should be discarded), and if
the value is 0 then the mask is the same as error_reporting - i.e.
it can be more dynamic and account for @, etc. and all non-reported
errors will be discarded. Fatal errors will never be discarded.
Alternative approach may be to just discard all non-reported errors,
but that could be a problem with user error handlers and extension-
supplied error callbacks, so the proposed approach is more flexible
as you could control discarding and reporting separately.So, what do you think?
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Index: main/main.c--- main/main.c (revision 287183)
+++ main/main.c (working copy)
@@ -640,6 +640,10 @@
char *message;
int is_function = 0;
if(!ZEND_CAN_REPORT(type)) {
return;}
/* get error text into buffer and escape for html if necessary */
buffer_len = vspprintf(&buffer, 0, format, args);
if (PG(html_errors)) {
@@ -827,6 +831,9 @@
char *params;
va_list args;if(!ZEND_CAN_REPORT(type)) {
return;}
spprintf(¶ms, 0, "%s,%s", param1, param2);
va_start(args, format);
php_verror(docref, params ? params : "...", type, format, args
TSRMLS_CC);
Index: Zend/zend.c
===================================================================
--- Zend/zend.c (revision 287462)
+++ Zend/zend.c (working copy)
@@ -76,6 +76,17 @@
}
/* }}} */+static ZEND_INI_MH(OnUpdateErrorMask) /* {{{ */
+{
- if (!new_value) {
EG(orig_error_reporting) = EG(error_reporting);- } else {
EG(orig_error_reporting) = atoi(new_value);- }
- return SUCCESS;
+}
+/* }}} */static ZEND_INI_MH(OnUpdateGCEnabled) /* {{{ */
{
OnUpdateBool(entry, new_value, new_value_length, mh_arg1, mh_arg2,
mh_arg3, stage TSRMLS_CC);
@@ -90,6 +101,7 @@ZEND_INI_BEGIN()
ZEND_INI_ENTRY("error_reporting", NULL, ZEND_INI_ALL,
OnUpdateErrorReporting)
ZEND_INI_ENTRY("error_mask", "-1", ZEND_INI_ALL,
OnUpdateErrorMask)
STD_ZEND_INI_BOOLEAN("zend.enable_gc", "1", ZEND_INI_ALL,
OnUpdateGCEnabled, gc_enabled, zend_gc_globals,
gc_globals)
#ifdef ZEND_MULTIBYTE
STD_ZEND_INI_BOOLEAN("detect_unicode", "1", ZEND_INI_ALL,
OnUpdateBool, detect_unicode, zend_compiler_globals, compiler_globals)
@@ -971,6 +983,10 @@
zend_class_entry *saved_class_entry;
TSRMLS_FETCH();if(!ZEND_CAN_REPORT(type)) {
return;}
/* Obtain relevant filename and lineno */
switch (type) {
case E_CORE_ERROR:
Index: Zend/zend.h
===================================================================
--- Zend/zend.h (revision 287462)
+++ Zend/zend.h (working copy)
@@ -766,6 +766,13 @@
ZEND_API void zend_replace_error_handling(zend_error_handling_t
error_handling, zend_class_entry *exception_class,
zend_error_handling *current TSRMLS_DC);
ZEND_API void zend_restore_error_handling(zend_error_handling *saved
TSRMLS_DC);+#define ZEND_FATAL_ERROR_MASK (E_CORE_ERROR|E_PARSE|E_COMPILE_ERROR|
E_ERROR|E_USER_ERROR)
+#define ZEND_CAN_REPORT(type) (\
- ((type & ZEND_FATAL_ERROR_MASK) != 0) || \
- (EG(orig_error_reporting) == 0 && (type & EG(error_reporting)) !=
- || \
- (type & EG(orig_error_reporting)) != 0
+)#endif /* ZEND_H */
/*
Hi!
While the patch is certainly interesting, I think masking errors is a
really really bad idea.
Any argument as to why?
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Well, couple of reasons:
-
Once you completely hide an error there is absolutely no trace what
so ever that it has even occurred, even if its the slowdown of the
native error handler being. -
Debugging code becomes that much more trickier, since a single
option effectively turns off PHP's error handling completely.
As far as I can tell the only benefit of this patch is to allow error
prone code to run faster by avoiding the slow down of generating error
message strings when errors occurs, but the downside is abuse by
people who just want errors to go away... IMHO the downside outweighs
the upside.
Hi!
While the patch is certainly interesting, I think masking errors is
a really really bad idea.Any argument as to why?
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
- Once you completely hide an error there is absolutely no trace what
so ever that it has even occurred, even if its the slowdown of the
native error handler being.
That's the point. And how the slowdown would be useful?
- Debugging code becomes that much more trickier, since a single option
effectively turns off PHP's error handling completely.
That's not correct. It doesn't turn off php error handling - it allows
you to specify which errors you handle and which not. That's what
error_reporting should be doing from the start, but it does not. Of
course, if you want notices, etc. to be displayed - display them, nobody
prevents you from doing so and you can see the default is very
permissive. But right now there's no way to save the busywork that
happens on errors that you don't want to be displayed. How it helps
your debugging if PHP allocates a bunch of memory, spends the cycles and
throws out the result without ever keeping it anywhere?
As far as I can tell the only benefit of this patch is to allow error
prone code to run faster by avoiding the slow down of generating error
message strings when errors occurs, but the downside is abuse by people
who just want errors to go away... IMHO the downside outweighs the upside.
This is not correct. There is a lot of code that deliberately ignores
errors - with @, etc. - because in that particular context these errors
are not useful. However there is no way to have the engine not to do all
the work it does on the errors - even if you know you don't want this
error to be displayed. There are a lot of completely legitimate cases
where you want some errors temporary or permanently suppressed - but
right now you don't have any means to do that, since error_reporting
does not suppress the whole generation process, but only the very last
stage of reporting. Which, frankly, makes very little sense - if you
don't ever want to report it, why generate it?
As for "abuse" - by which you mean running code with errors disabled I
guess - everybody that ever ran third-party code in production knows
that it is an everyday reality that a lot of code out there is not
E_NOTICE-safe, even more is not E_STRICT/E_DEPRECATED/etc. safe. And
production settings - including ones we recommend - do not include
reporting these errors. So they serve absolutely no useful purpose but
spending cycles on useless work - humans never see them, nobody debugs
them, only thing they do is waste time.
So one may preach that shouldn't happen as much as one wants, and call
it "abuse" as much as one wants, but that's what happens and that's what
will always happen. The question is only if people's sites would run
slow or run faster. I don't see any gain in denying people means to run
their sites faster.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
- Once you completely hide an error there is absolutely no trace
what so ever that it has even occurred, even if its the slowdown of
the native error handler being.That's the point. And how the slowdown would be useful?
Because it may force to fix the errors, rather then ignore them?
- Debugging code becomes that much more trickier, since a single
option effectively turns off PHP's error handling completely.That's not correct. It doesn't turn off php error handling - it
allows you to specify which errors you handle and which not. That's
what error_reporting should be doing from the start, but it does
not. Of course, if you want notices, etc. to be displayed - display
them, nobody prevents you from doing so and you can see the default
is very permissive. But right now there's no way to save the
busywork that happens on errors that you don't want to be
displayed. How it helps your debugging if PHP allocates a bunch of
memory, spends the cycles and throws out the result without ever
keeping it anywhere?As far as I can tell the only benefit of this patch is to allow
error prone code to run faster by avoiding the slow down of
generating error message strings when errors occurs, but the
downside is abuse by people who just want errors to go away... IMHO
the downside outweighs the upside.This is not correct. There is a lot of code that deliberately
ignores errors - with @
There is very little modern code that is @ ridden like the stuff
written before. The way to have the engine not do the work, would be
as simple as fixing the code, which is the right solution here.
As for "abuse" - by which you mean running code with errors disabled
I guess - everybody that ever ran third-party code in production
knows that it is an everyday reality that a lot of code out there is
not E_NOTICE-safe, even more is not E_STRICT/E_DEPRECATED/etc. safe.
The production value we use according to php.ini-production is E_ALL &
~E_DEPRECATED, which means E_NOTICE will not be hidden.
And production settings - including ones we recommend - do not
include reporting these errors. So they serve absolutely no useful
purpose but spending cycles on useless work - humans never see them,
nobody debugs them, only thing they do is waste time.
Well, if you are going to hide them, the no one will ever debug them,
let alone know that they exist.
Hi!
Because it may force to fix the errors, rather then ignore them?
Come on. That doesn't make any sense. If people deliberately disabled
the error, they don't want to see it, and they will not see it. Making
it slow doesn't do anything useful. Let's insert some sleep()'s into
zend_error() too, would it make it better? Let's make E_DEPRECATED take
5 seconds, that'll show them.
Making engine work slow to force people into some behavior is very
misguided idea. I can't believe it is proposed to make invisible
slowdowns in unknown places as a means to make people write better code.
There is very little modern code that is @ ridden like the stuff written
If we discard emotional terms like "ridden" - there is such code in
almost every major app out there. Just search for @fopen or @include or
@xml or @eval - tons of examples. Ans the reason is simple - people do
not always want PHP to display/log errors for them. Sometimes they don't
care for the errors, sometimes they want to handle the error by other
means.
before. The way to have the engine not do the work, would be as simple
as fixing the code, which is the right solution here.
What you still can't see if that this code is not broken, there's
nothing to fix - this code is EXPLICITLY saying "do this, and if there's
a error - ignore it". That's what the author wants. It's not an omission
or mistake - it's intentional, it's part of the requirements. Only
error_reporting now is broken so ignored errors waste excessive time.
The right solution here is to fix error system so ignored errors not
waste excessive time.
The production value we use according to php.ini-production is
E_ALL&
~E_DEPRECATED, which meansE_NOTICEwill not be hidden.
Which conveniently ignores my argument about all other error types and
bigger argument that THERE ARE such types and people ignore them on
purpose, not because they are stupid.
Well, if you are going to hide them, the no one will ever debug them,
let alone know that they exist.
How one would know they exist if reporting is disabled? By guessing it
from the slowdown? I can't believe deliberately slowing down user
applications in unknown places is really being recommended as a way of
debugging.
The fact is we have error_reporting and @, and however you may hate
them, they exist and they are used. My patch only makes them do what
they should be doing from the start, while keeping BC and allowing a bit
more flexibility in advanced cases where you have special needs (like
monitoring system that collects unreported errors, etc.).
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
There is very little modern code that is @ ridden like the stuff
writtenIf we discard emotional terms like "ridden" - there is such code in
almost every major app out there. Just search for @fopen or @include
or @xml or @eval - tons of examples. Ans the reason is simple -
people do not always want PHP to display/log errors for them.
Sometimes they don't care for the errors, sometimes they want to
handle the error by other means.
Unless your code is "ridden" or if you prefer "filled" with @ and/or
errors the speed improvement would be next to impossible to measure
since you'd be literally saving microseconds. You'd need to have a
substantial chunk of your code generating errors for your
optimizations to make a reliably measurable difference. More over, if
the user defines an error handler, which many applications,
frameworks, etc... do then your improvement is next to invisible in
between the overhead of executing PHP code to process the error.
before. The way to have the engine not do the work, would be as
simple as fixing the code, which is the right solution here.What you still can't see if that this code is not broken, there's
nothing to fix - this code is EXPLICITLY saying "do this, and if
there's a error - ignore it".
Vast majority of E_NOTICEs are not fixed not because people don't want
to, but rather due to the fact that for ages PHP's default settings
were set to ignore those errors. And E_NOTICES btw often could've let
people know about security faults before they were abused, such as
uninitialized variables and array keys that could be injected via
register_globals, extract() function, etc...
Stas, my biggest issue is that you are making this configurable value
that makes this open to abuse, rather then using 0 as default and
matching the error message creation to error reporting settings.
ILia
Hi!
Unless your code is "ridden" or if you prefer "filled" with @ and/or
errors the speed improvement would be next to impossible to measure
since you'd be literally saving microseconds. You'd need to have a
Microsecond here, microsecond there - this stuff adds up. And 2-3
mallocs of substantial size (IIRC some messages are long enough so
emalloc caches do not apply) plus whole printf ordeal - even twice in
some cases - are not that small change, especially if it could happen
repeatedly. Of course it alone won't make you 50% speedup, but even 0.5%
here and 0.5% there add up.
BTW, if you benchmark only this thing (yes, I know, it's not realistic
:) the difference is very, very measurable - error reporting slows down
operation with ignored error 3-4 times. I think if I proposed an
improvement that would speed up certain set of opcodes 3 times that'd be
worth having, not?
to make a reliably measurable difference. More over, if the user defines
an error handler, which many applications, frameworks, etc... do then
your improvement is next to invisible in between the overhead of
executing PHP code to process the error.
The thing is you would be in control of which errors too feed to the
error handler and which not to. That's exactly the point - giving you
the tools to control it. Including tools to deal with "noisy" code the
way you like - which may be your way, my way or any way in between. If
you don't care - default changes nothing, but if you do, you have the
means to make it run faster.
Vast majority of E_NOTICEs are not fixed not because people don't want
You are still focused on one particular case of E_NOTICE. It's not
(only) what's it about, it's bigger.
set to ignore those errors. And E_NOTICES btw often could've let people
know about security faults before they were abused, such as
uninitialized variables and array keys that could be injected via
register_globals,extract()function, etc...
Yes, yes - if only they were actually seen by anybody. The case we
discuss here is when they are not seen anyway. So I don't see how your
argument applies. Your argument is about entirely different thing which
I do not argue with you about - that some warning should not be
disabled/hidden. I agree with you. But some OTHER warnings should be,
and that's the case I want to fix.
Stas, my biggest issue is that you are making this configurable value
that makes this open to abuse, rather then using 0 as default and
matching the error message creation to error reporting settings.
It can be done too, but not having other configuration will disable some
scenarios when you don't want the error to go through usual reporting
mechanisms but want debugger/monitoring system still see it. I
understand that if we designed error_reporting from scratch maybe we'd
just say "one switch for everybody", but since we already have people
that are used to having current system, I made it as flexible as
possible. If people around here think it's too flexible I could just
make it on/off (i.e. same as -1/0 with current patch), it's trivial. I
just wanted to start with max flexibility.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Quite boring to read this thread where two persons argue about something
abstract. Stas, can you give a real life example where your patch is necessary..?
--Jani
Stanislav Malyshev wrote:
Hi!
Unless your code is "ridden" or if you prefer "filled" with @ and/or
errors the speed improvement would be next to impossible to measure
since you'd be literally saving microseconds. You'd need to have aMicrosecond here, microsecond there - this stuff adds up. And 2-3
mallocs of substantial size (IIRC some messages are long enough so
emalloc caches do not apply) plus whole printf ordeal - even twice in
some cases - are not that small change, especially if it could happen
repeatedly. Of course it alone won't make you 50% speedup, but even 0.5%
here and 0.5% there add up.
BTW, if you benchmark only this thing (yes, I know, it's not realistic
:) the difference is very, very measurable - error reporting slows down
operation with ignored error 3-4 times. I think if I proposed an
improvement that would speed up certain set of opcodes 3 times that'd be
worth having, not?to make a reliably measurable difference. More over, if the user
defines an error handler, which many applications, frameworks, etc...
do then your improvement is next to invisible in between the overhead
of executing PHP code to process the error.The thing is you would be in control of which errors too feed to the
error handler and which not to. That's exactly the point - giving you
the tools to control it. Including tools to deal with "noisy" code the
way you like - which may be your way, my way or any way in between. If
you don't care - default changes nothing, but if you do, you have the
means to make it run faster.Vast majority of E_NOTICEs are not fixed not because people don't want
You are still focused on one particular case of E_NOTICE. It's not
(only) what's it about, it's bigger.set to ignore those errors. And E_NOTICES btw often could've let
people know about security faults before they were abused, such as
uninitialized variables and array keys that could be injected via
register_globals,extract()function, etc...Yes, yes - if only they were actually seen by anybody. The case we
discuss here is when they are not seen anyway. So I don't see how your
argument applies. Your argument is about entirely different thing which
I do not argue with you about - that some warning should not be
disabled/hidden. I agree with you. But some OTHER warnings should be,
and that's the case I want to fix.Stas, my biggest issue is that you are making this configurable value
that makes this open to abuse, rather then using 0 as default and
matching the error message creation to error reporting settings.It can be done too, but not having other configuration will disable some
scenarios when you don't want the error to go through usual reporting
mechanisms but want debugger/monitoring system still see it. I
understand that if we designed error_reporting from scratch maybe we'd
just say "one switch for everybody", but since we already have people
that are used to having current system, I made it as flexible as
possible. If people around here think it's too flexible I could just
make it on/off (i.e. same as -1/0 with current patch), it's trivial. I
just wanted to start with max flexibility.
I think we should do something along the lines of what Stas is
suggesting. The current approach of allocating and sprintf'ing all
messages regardless of whether they will ever be used for anything is
painful and a huge impediment to adding informative E_NOTICE and
E_STRICT messages in the future.
A good example of the benefit of this patch is upgrading a server
running older PHP code that causes a lot of E_STRICT messages. The
performance hit can be significant to the point where people may
downgrade to the older version until they can get around to cleaning up
the code. Having the ability to properly turn off E_STRICT such that
not only are E_STRICT messages not shown, but they also don't eat up
valuable cpu cycles is something we should have done long ago. It is
non-intuitive that disabling an error type doesn't also remove the
performance hit associated with generating that error message.
-Rasmus
Jani Taskinen wrote:
Quite boring to read this thread where two persons argue about something
abstract. Stas, can you give a real life example where your patch is
necessary..?--Jani
Stanislav Malyshev wrote:
Hi!
Unless your code is "ridden" or if you prefer "filled" with @ and/or
errors the speed improvement would be next to impossible to measure
since you'd be literally saving microseconds. You'd need to have aMicrosecond here, microsecond there - this stuff adds up. And 2-3
mallocs of substantial size (IIRC some messages are long enough so
emalloc caches do not apply) plus whole printf ordeal - even twice in
some cases - are not that small change, especially if it could happen
repeatedly. Of course it alone won't make you 50% speedup, but even
0.5% here and 0.5% there add up.
BTW, if you benchmark only this thing (yes, I know, it's not realistic
:) the difference is very, very measurable - error reporting slows
down operation with ignored error 3-4 times. I think if I proposed an
improvement that would speed up certain set of opcodes 3 times that'd
be worth having, not?to make a reliably measurable difference. More over, if the user
defines an error handler, which many applications, frameworks, etc...
do then your improvement is next to invisible in between the overhead
of executing PHP code to process the error.The thing is you would be in control of which errors too feed to the
error handler and which not to. That's exactly the point - giving you
the tools to control it. Including tools to deal with "noisy" code the
way you like - which may be your way, my way or any way in between. If
you don't care - default changes nothing, but if you do, you have the
means to make it run faster.Vast majority of E_NOTICEs are not fixed not because people don't want
You are still focused on one particular case of E_NOTICE. It's not
(only) what's it about, it's bigger.set to ignore those errors. And E_NOTICES btw often could've let
people know about security faults before they were abused, such as
uninitialized variables and array keys that could be injected via
register_globals,extract()function, etc...Yes, yes - if only they were actually seen by anybody. The case we
discuss here is when they are not seen anyway. So I don't see how
your argument applies. Your argument is about entirely different thing
which I do not argue with you about - that some warning should not be
disabled/hidden. I agree with you. But some OTHER warnings should be,
and that's the case I want to fix.Stas, my biggest issue is that you are making this configurable value
that makes this open to abuse, rather then using 0 as default and
matching the error message creation to error reporting settings.It can be done too, but not having other configuration will disable
some scenarios when you don't want the error to go through usual
reporting mechanisms but want debugger/monitoring system still see it.
I understand that if we designed error_reporting from scratch maybe
we'd just say "one switch for everybody", but since we already have
people that are used to having current system, I made it as flexible
as possible. If people around here think it's too flexible I could
just make it on/off (i.e. same as -1/0 with current patch), it's
trivial. I just wanted to start with max flexibility.
2009/8/24 Rasmus Lerdorf rasmus@lerdorf.com:
I think we should do something along the lines of what Stas is
suggesting. The current approach of allocating and sprintf'ing all
messages regardless of whether they will ever be used for anything is
painful and a huge impediment to adding informativeE_NOTICEand
E_STRICTmessages in the future.A good example of the benefit of this patch is upgrading a server
running older PHP code that causes a lot ofE_STRICTmessages. The
performance hit can be significant to the point where people may
downgrade to the older version until they can get around to cleaning up
the code. Having the ability to properly turn offE_STRICTsuch that
not only areE_STRICTmessages not shown, but they also don't eat up
valuable cpu cycles is something we should have done long ago. It is
non-intuitive that disabling an error type doesn't also remove the
performance hit associated with generating that error message.-Rasmus
As an outsider, and from what I've read here, surely the solution is
to simply move the test to determine if an error type is reportable.
If anything, as you ARE all saying that there is a slowdown, I
should maybe file a bug ...
"My @ridden code is really slow, even when I turn off error_reporting!"
So, as an outsider, I'm +1 for speeding up PHP by not running
unnecessary code, but -1 for introducing what is surely quite
obviously an unnecessary ini entry.
<speech style=iInspirational">Essentially, it's a bug folks! So let's
fix it!</speech>
Regards,
Richard.
--
Richard Quadling
"Standing on the shoulders of some very clever giants!"
EE : http://www.experts-exchange.com/M_248814.html
Zend Certified Engineer : http://zend.com/zce.php?c=ZEND002498&r=213474731
ZOPA : http://uk.zopa.com/member/RQuadling
Hi!
Quite boring to read this thread where two persons argue about something
abstract. Stas, can you give a real life example where your patch is
necessary..?
Any code where you either use @ or error_reporting which is not -1 would
benefit from it by not processing errors that go nowhere. I just looked
at Zend Framework - with is pretty clean with regard to
E_NOTICE/E_STRICT problems - and @ is used in dozens of classes around.
The speedup would be probably not very big for whole RL application, but
it's a 10-line patch, and little things help too.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
Quite boring to read this thread where two persons argue about
something abstract. Stas, can you give a real life example where
your patch is necessary..?Any code where you either use @ or error_reporting which is not -1
would benefit from it by not processing errors that go nowhere. I
just looked at Zend Framework - with is pretty clean with regard to
E_NOTICE/E_STRICT problems - and @ is used in dozens of classes
around. The speedup would be probably not very big for whole RL
application, but it's a 10-line patch, and little things help too.
well a few of those places would probably be fixable, by providing
functions to check beforehand if calling the final function would
cause an error. but that if course would add more overhead, but would
still be the "cleaner" solution.
overall i am not so convinced about the ignoring approach. as for
E_STRICT .. that shouldnt become less of an issue now that we have
E_DEPRECATED .. but i guess that just means that in the future people
will complain about E_DEPRECATED ..
anyways to me both E_STRICT and E_DEPRECATED are development tools
that can be totally ignored in production. however E_NOTICE should not
occur in production and we shouldnt encourage people to make them
disappear entirely.
regards,
Lukas Kahwe Smith
mls@pooteeweet.org
Lukas Kahwe Smith wrote:
Hi!
Quite boring to read this thread where two persons argue about
something abstract. Stas, can you give a real life example where your
patch is necessary..?Any code where you either use @ or error_reporting which is not -1
would benefit from it by not processing errors that go nowhere. I just
looked at Zend Framework - with is pretty clean with regard to
E_NOTICE/E_STRICT problems - and @ is used in dozens of classes
around. The speedup would be probably not very big for whole RL
application, but it's a 10-line patch, and little things help too.well a few of those places would probably be fixable, by providing
functions to check beforehand if calling the final function would cause
an error. but that if course would add more overhead, but would still be
the "cleaner" solution.overall i am not so convinced about the ignoring approach. as for
E_STRICT.. that shouldnt become less of an issue now that we have
E_DEPRECATED.. but i guess that just means that in the future people
will complain aboutE_DEPRECATED..anyways to me both
E_STRICTandE_DEPRECATEDare development tools that
can be totally ignored in production. howeverE_NOTICEshould not occur
in production and we shouldnt encourage people to make them disappear
entirely.
Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED,
E_NOTICE, whatever, all cause a performance hit even if the
error_reporting level is such that they will never show up anywhere.
That's what this patch is trying to address. To write optimal code,
they have to be entirely clean of all messages including E_DEPRECATED
and E_STRICT.
-Rasmus
Lukas Kahwe Smith wrote:
Hi!
Quite boring to read this thread where two persons argue about
something abstract. Stas, can you give a real life example where
your
patch is necessary..?Any code where you either use @ or error_reporting which is not -1
would benefit from it by not processing errors that go nowhere. I
just
looked at Zend Framework - with is pretty clean with regard to
E_NOTICE/E_STRICT problems - and @ is used in dozens of classes
around. The speedup would be probably not very big for whole RL
application, but it's a 10-line patch, and little things help too.well a few of those places would probably be fixable, by providing
functions to check beforehand if calling the final function would
cause
an error. but that if course would add more overhead, but would
still be
the "cleaner" solution.overall i am not so convinced about the ignoring approach. as for
E_STRICT.. that shouldnt become less of an issue now that we have
E_DEPRECATED.. but i guess that just means that in the future people
will complain aboutE_DEPRECATED..anyways to me both
E_STRICTandE_DEPRECATEDare development tools
that
can be totally ignored in production. howeverE_NOTICEshould not
occur
in production and we shouldnt encourage people to make them disappear
entirely.Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED,
E_NOTICE, whatever, all cause a performance hit even if the
error_reporting level is such that they will never show up anywhere.
That's what this patch is trying to address. To write optimal code,
they have to be entirely clean of all messages includingE_DEPRECATED
and E_STRICT.
right .. what i was trying to say was that i can see value in being
able to hide E_DEPRECATED and E_STRICT, but the error stuff shouldnt
be hidden that way. users should either display or log their E_NOTICES
and fix them!
regards,
Lukas Kahwe Smith
mls@pooteeweet.org
Hi!
Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED,
E_NOTICE, whatever, all cause a performance hit even if the
error_reporting level is such that they will never show up anywhere.
That's what this patch is trying to address. To write optimal code,
they have to be entirely clean of all messages includingE_DEPRECATED
and E_STRICT.
BTW "cleaning the messages" won't help with performance problems in many
cases. Let's suppose you have a code that tries to open some file and if
it exists, do stuff (and if it doesn't it doesn't care):
$fp = @fopen($filename, "r");
if($fp) {
// do stuff
}
Now if you "clean" it, you do something like:
if(is_readable($filename)) {
$fp = fopen($filename, "r");
// do stuff
}
(and if you don't want race condition there, you still need the if()).
Then is_readable and fopen can still can't be guaranteed to not produce
warnings if there are open_basedir restrictions or streams involved or
FS could change in the meanitime. But now instead of one FS access, you
have two - not much of a speedup.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED,
E_NOTICE, whatever, all cause a performance hit even if the
error_reporting level is such that they will never show up anywhere.
That's what this patch is trying to address. To write optimal code,
they have to be entirely clean of all messages includingE_DEPRECATED
and E_STRICT.BTW "cleaning the messages" won't help with performance problems in
many cases. Let's suppose you have a code that tries to open some
file and if it exists, do stuff (and if it doesn't it doesn't care):
$fp = @fopen($filename, "r");
if($fp) {
// do stuff
}
Now if you "clean" it, you do something like:
if(is_readable($filename)) {
$fp = fopen($filename, "r");
// do stuff
}
(and if you don't want race condition there, you still need the
if()). Then is_readable and fopen can still can't be guaranteed to
not produce warnings if there are open_basedir restrictions or
streams involved or FS could change in the meanitime. But now
instead of one FS access, you have two - not much of a speedup.
right .. but it should reduce the chances of such errors occurring.
and like i said .. that is obviously not a speed up, but a slow down.
but i generally think its wise to encourage people to properly avoid
error conditions locally, because otherwise sooner rather than later
you will also no longer see the error conditions which you did not try
to avoid/handle locally.
regards,
Lukas Kahwe Smith
mls@pooteeweet.org
Lukas Kahwe Smith wrote:
anyways to me both
E_STRICTandE_DEPRECATEDare development tools that
can be totally ignored in production. howeverE_NOTICEshould not occur
in production and we shouldnt encourage people to make them disappear
entirely.Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED,
E_NOTICE, whatever, all cause a performance hit even if the
error_reporting level is such that they will never show up anywhere.
That's what this patch is trying to address. To write optimal code,
they have to be entirely clean of all messages includingE_DEPRECATED
and E_STRICT.
And how exactly is that a problem? Sure, there are some cases where PHP
functions are too noisy, but that should be addressed instead.
regards,
Derick
--
http://derickrethans.nl | http://ezcomponents.org | http://xdebug.org
twitter: @derickr
Derick Rethans wrote:
Lukas, the problem is that all messages, E_STRICT, E_DEPRECATED,
E_NOTICE, whatever, all cause a performance hit even if the
error_reporting level is such that they will never show up anywhere.
That's what this patch is trying to address. To write optimal code,
they have to be entirely clean of all messages includingE_DEPRECATED
and E_STRICT.And how exactly is that a problem? Sure, there are some cases where PHP
functions are too noisy, but that should be addressed instead.
You are making the assumption that everybody agrees on E_NOTICE,
E_STRICT and E_DEPRECATED being things to be fixed in your code. That is
not the case and punishing people for not following exactly the same
coding style as you doesn't make sense when it could easily be avoided.
Background info: We decided to patch PHP to not generate E_NOTICE for
undefined variables or (worse) array offsets because we use this
language feature a lot and cannot afford the overhead caused by
E_NOTICEs even if error_reporting is set to ignore E_NOTICE. And you
won't be able to convince us to change this policy because being able to
do things like
if (!$visited[$city]++)
is more valuable (i.e. keeps the code uncluttered) to us than the
E_NOTICE. We use http://cschneid.com/php/phplint.php (in a commit hook)
instead to catch typos. It's not perfect but does the job for us.
Summary: Don't punish different coding styles unless really necessary.
Cheers,
- Chris
hi Lukas,
Hi!
Quite boring to read this thread where two persons argue about something
abstract. Stas, can you give a real life example where your patch is
necessary..?Any code where you either use @ or error_reporting which is not -1 would
benefit from it by not processing errors that go nowhere. I just looked at
Zend Framework - with is pretty clean with regard to E_NOTICE/E_STRICT
problems - and @ is used in dozens of classes around. The speedup would be
probably not very big for whole RL application, but it's a 10-line patch,
and little things help too.well a few of those places would probably be fixable, by providing functions
to check beforehand if calling the final function would cause an error. but
that if course would add more overhead, but would still be the "cleaner"
solution.
To call a function to avoid a noisy or resource expensive error is just as bad.
There are many common operations in PHP where we over react (file ops
for example). As I agree with Rasmus' comments, I do think that this
patch is the wrong fix for the real problem (that does not mean I do
not like to have it in). I would however prefer to have APIs without
any kind of noisy errors, at least for the common operations or where
the error is obviously expected (as IO can fail). Please note that I
did not suggest to use exceptions everywhere.
Cheers,
Pierre
Hi!
Quite boring to read this thread where two persons argue about
something abstract. Stas, can you give a real life example where your
patch is necessary..?Any code where you either use @ or error_reporting which is not -1 would
benefit from it by not processing errors that go nowhere. I just looked
at Zend Framework - with is pretty clean with regard to
E_NOTICE/E_STRICT problems - and @ is used in dozens of classes around.
The speedup would be probably not very big for whole RL application, but
it's a 10-line patch, and little things help too.
Just wondering why nobody hasn't suggested the obvious (?) alternative
yet: Why not fix error_reporting to work like one expects. As in: If an
error level isn't in it, then nothing happens. Adding an extra option to
do that seems a bit overkill..
--Jani
Hi!
Just wondering why nobody hasn't suggested the obvious (?) alternative
yet: Why not fix error_reporting to work like one expects. As in: If an
error level isn't in it, then nothing happens. Adding an extra option to
do that seems a bit overkill..
Because it would break other funcions (namely, $php_errormsg, error
handlers, etc.) which may be used by some.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
Just wondering why nobody hasn't suggested the obvious (?) alternative
yet: Why not fix error_reporting to work like one expects. As in: If
an error level isn't in it, then nothing happens. Adding an extra
option to do that seems a bit overkill..Because it would break other funcions (namely, $php_errormsg, error
handlers, etc.) which may be used by some.
Well, not necessarily. How doesn't your patch break them? Just do the
same but without adding new option.
--Jani
Hi,
I think the idea is good intended but will cause troubles of introduced in
5.3. I alone have a bunch of code with custom error handlers that expect to
receive all errors at all times. If the feature allows runtime tweaking via
ini_set/get, then one could add more plumbing code and workaround, but it
still doesn't make it BC to the expected behavior for existing 5.x apps.
However I fully agree E_NOTICE should:
- be reserved for informational may-be-bad-but-usually-is-ok messages
- not have performance impact when disabled (i.e. production)
The problem? Right now important errors that could cause data damage if
ignored are E_NOTICE instead of E_WARNING.
I'm talking E_NOTICE when you using variables that don't exist, missing
constants and so on. All of this should be E_WARNING so my production code
can stop and notify me when it's encountered, then I wouldn't mind filtering
E_NOTICE.
Also Stas, consider why are you trying to treat the symptom of unwanted
errors slowing things down. I would bet it's something like the stream APIs
emitting errors on 404 response and such. Some API-s really throw notices
and warnings where it makes no sense, so people mute them out of necessity
(or use handlers). I'd rather fix the API-s instead of pretend it doesn't
happen by masking the errors.
All of the above would be a job for 6.0 IMHO, 5.x has pulled enough changes
under our feet and it's really not fun anymore :P
Regards,
Stan Vassilev
Hi Stas:
Just search for @fopen or @include or
@xml or @eval - tons of examples.
True. But this patch could cause problems.
@ is often used to stop error/warning output to the browser on that line,
but the next couple lines of code are used to handle that error. For
example:
if (!($dom = @DOMDocument::load($file_name))) {
log_it('invalid XML: ' . $php_errormsg);
die('invalid XML');
}
So if error processing is totally turned off, $php_errormsg won't be
populated.
I like the general idea, but I'm encouraging folks to carefully consider
the ramifications of various implementation methods.
Thanks,
--Dan
--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409
Hi!
@ is often used to stop error/warning output to the browser on that line,
but the next couple lines of code are used to handle that error. For
example:if (!($dom = @DOMDocument::load($file_name))) {
log_it('invalid XML: ' . $php_errormsg);
die('invalid XML');
}So if error processing is totally turned off, $php_errormsg won't be
populated.
That's true. So, if you use code that uses $php_errormsg, of course you
can not use this optimization and should not enable it (at least for
error types and code parts that you use $php_errormsg with).
Also, if you use @ to stop warning output to the browser you should read
the manual about display_errors and part of the security guidelines when
it says never enable display_errors in production ;)
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hey Stas:
That's true. So, if you use code that uses $php_errormsg, of course you
can not use this optimization and should not enable it (at least for
error types and code parts that you use $php_errormsg with).
Exactly. Totally killing E_STRICT on it's own seems like the biggest win
(in the right circumstances).
My main point is that we need to think this thing through carefully and
document it well.
Also, if you use @ to stop warning output to the browser you should read
the manual about display_errors and part of the security guidelines when
it says never enable display_errors in production ;)
Of course. But folks don't want those same messages showing up in the
error log, either.
Thanks,
--Dan
--
T H E A N A L Y S I S A N D S O L U T I O N S C O M P A N Y
data intensive web and database programming
http://www.AnalysisAndSolutions.com/
4015 7th Ave #4, Brooklyn NY 11232 v: 718-854-0335 f: 718-854-0409
Personally, I generally prefer log_errors and E_ALL in production.
That said, there have been cases where it really wouldn't make much
sense to log the errors on a web tier with many nodes.
Or, at least, it didn't make sense to just log the errors.
But I digress.
I believe Stas has made a good pragamatic argument for a reasonable
use case that improves performance in common enough usage by people
who do know what they are doing, and want/need the performance gain.
The detractors have, largely, not been cogent, to be frank.
The most reasonable have been to be careful not to rush this through
without considering its effects on cases like X. To which Stas says,
"Of course not. Don't use it in case X."
I must say, however, that buried in Stas' use case, 3rd party,
not-so-clean code, and realistic pragmatic installs thereof, is buried
the rather large problem:
Many people who do such a thing, do not really do a serious
code-review of the 3rd party code.
They toss it on there, it works, and they move on to the next task.
I foresee a fair number of people who turn on error_mask, and then are
defuddled by the 3rd party apps they never reviewed in the first place
not behaving.
Certainly, they are getting what they deserve, in some lights.
But, to remain pragmatic, the PHP Dev Team has to be ready for an
onslaught of issues and bug reports about 3rd party software in this
state.
As an example, before this feature rolls out, I'd have to suggest that
the bug reporting page include it in things to turn OFF before a bug
report is accepted, in that section about 3rd party extensions.
This is just one example off the top of my head. I'm pretty sure
there are more ramifications that go well beyond the code-based
discussion so far.
All that said:
I am in favor of this patch, provided sufficient effort to be sure the
community knows, in advance, loud and clear, it's not a cure-all and
is meant only for code that can't/won't be fixed, rather than code
that is in active development.
In fact, I'd suggest that using error_mask should emit an E_STRICT,
not suppressed by error_mask, before it kicks in. :-)
--
Some people ask for gifts here.
I just want you to buy an Indie CD for yourself:
http://cdbaby.com/search/from/lynch
I must say, however, that buried in Stas' use case, 3rd party,
not-so-clean code, and realistic pragmatic installs thereof, is buried
the rather large problem:Many people who do such a thing, do not really do a serious
code-review of the 3rd party code.
Right and even so .. why should using that 3rd party code make it
sensible to flip this switch on everything? I am not really suggesting
the following because I havent thought it out .. just an example of
out of the box thinking we need in order to try and really address the
root causes. Maybe we rather need some way to define error handling on
a per directory basis. This way you could just mark this 3rd party lib
to use a different error reporting mode. Maybe this could be expanded
even more to also cover the ever increasing number of extensions that
optionally throw exceptions. This way we might even be able to
reconcile the issue that if you run PDO and one lib wants exception
mode and the other doesnt, you need two PDO instances.
Another, again not fully thought out approach, would be to do
something along the lines of exceptions. A magic object which can
carry some more context but still returns true on a === false check.
This object would of course have to be super lightweight and it should
just contain the necessary bits in order to be able to generate the
full error message if needed. I guess that would still probably not
solve the performance "issue" that Stas is trying to solve, since its
hard to imagine that such a solution would not come with so much
overhead that it speed up anything. However it might provide for a
better solution to deal with providing error context information
without forcing everybody to deal with try/catch.
Anyways, so yeah I am squarely in the camp that believes that adding
more tools to just forget about error reporting is bad mojo.
I also very much agree that many functions do not use the right
reporting level and that several functions could probably be made to
make it easier to avoid those reports to begin with, though yes there
are cases where there is simply no way to avoid them.
regards,
Lukas Kahwe Smith
mls@pooteeweet.org
Hi!
I foresee a fair number of people who turn on error_mask, and then are
defuddled by the 3rd party apps they never reviewed in the first place
not behaving.
/.../
All that said:
I am in favor of this patch, provided sufficient effort to be sure the
community knows, in advance, loud and clear, it's not a cure-all and
is meant only for code that can't/won't be fixed, rather than code
that is in active development.
As I already noted, the masking - in most cases and definitely in
recommended cases - would happen for errors that are NOT SEEN. Not
reported. Not logged. Before the patch. Which means, whatever advantage
you seek from looking at these errors, fixing them, reviewing the code,
doing anything related to them at all, etc., etc. - you have ALREADY
lost if when you decided not to report these errors. Without the patch.
Before the patch. So all arguments about how wrong it is to disable
errors when errors in fact might be useful are, again, irrelevant - they
are already disabled without the patch.
So far, the argument that made the most sense on this topic is that
using this patch would taint you with "bad mojo", I guess because when
you sacrifice some performance to the Gods of Unreported Errors, it's
all OK, but without that sacrifice, they could become enraged and
revenge you by... I don't know, giving you more bad mojo?
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
As I already noted, the masking - in most cases and definitely in
recommended cases - would happen for errors that are NOT SEEN. Not
reported. Not logged. Before the patch. Which means, whatever
advantage you seek from looking at these errors, fixing them,
reviewing the code, doing anything related to them at all, etc.,
etc. - you have ALREADY lost if when you decided not to report these
errors. Without the patch. Before the patch. So all arguments about
how wrong it is to disable errors when errors in fact might be
useful are, again, irrelevant - they are already disabled without
the patch.
So far, the argument that made the most sense on this topic is that
using this patch would taint you with "bad mojo", I guess because
when you sacrifice some performance to the Gods of Unreported
Errors, it's all OK, but without that sacrifice, they could become
enraged and revenge you by... I don't know, giving you more bad mojo?
Of course we are well aware that you can choose to ignore errors even
today. However instead of adding yet another ini setting, some of us
feel we should rather focus on solving the real issues. That certain
errors in certain parts of your code are unavoidable and known and
that certain pieces of code will raise errors to the global error
handler even though you have all the code in place to handle the issue
locally.
regards,
Lukas Kahwe Smith
mls@pooteeweet.org
Hi!
Of course we are well aware that you can choose to ignore errors even
today. However instead of adding yet another ini setting, some of us
feel we should rather focus on solving the real issues. That certain
This is a real issue. You have better solution for it? Go ahead, propose
it. So far best I saw is useless discussion about "somehow fixing all
functions not to report errors" and even more useless discussions about
moving to exceptions, etc. And the bad mojo stuff, of course.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Lukas Kahwe Smith wrote:
As I already noted, the masking - in most cases and definitely in
recommended cases - would happen for errors that are NOT SEEN. Not
reported. Not logged. Before the patch. Which means, whatever
advantage you seek from looking at these errors, fixing them,
reviewing the code, doing anything related to them at all, etc., etc.
- you have ALREADY lost if when you decided not to report these
errors. Without the patch. Before the patch. So all arguments about
how wrong it is to disable errors when errors in fact might be useful
are, again, irrelevant - they are already disabled without the patch.
So far, the argument that made the most sense on this topic is that
using this patch would taint you with "bad mojo", I guess because when
you sacrifice some performance to the Gods of Unreported Errors, it's
all OK, but without that sacrifice, they could become enraged and
revenge you by... I don't know, giving you more bad mojo?Of course we are well aware that you can choose to ignore errors even
today. However instead of adding yet another ini setting, some of us
feel we should rather focus on solving the real issues. That certain
errors in certain parts of your code are unavoidable and known and that
certain pieces of code will raise errors to the global error handler
even though you have all the code in place to handle the issue locally.
But that isn't actually the real issue.
There will always be a reason to not report certain types of errors.
E_STRICT and E_DEPRECATED are informative development-level messages
that have no place on a production system. They should be turned off.
The actual real problem is that our error mechanism only allows us to
turn off the final output stage, but not the actual generation of those
errors. That is the real issue.
I do agree that having another ini switch isn't ideal, but I have
pondered this a few times and I don't see a solution that doesn't
involve some sort of configuration like that because of custom error
handlers and the php_errormsg issues.
-Rasmus
Hi!
I foresee a fair number of people who turn on error_mask, and then
are
defuddled by the 3rd party apps they never reviewed in the first
place
not behaving.
/.../
All that said:
I am in favor of this patch, provided sufficient effort to be sure
the
community knows, in advance, loud and clear, it's not a cure-all
and
is meant only for code that can't/won't be fixed, rather than code
that is in active development.As I already noted, the masking - in most cases and definitely in
recommended cases - would happen for errors that are NOT SEEN. Not
reported. Not logged. Before the patch. Which means, whatever
advantage
you seek from looking at these errors, fixing them, reviewing the
code,
doing anything related to them at all, etc., etc. - you have ALREADY
lost if when you decided not to report these errors. Without the
patch.
Before the patch. So all arguments about how wrong it is to disable
errors when errors in fact might be useful are, again, irrelevant -
they
are already disabled without the patch.
I understand, really I do...
And I know you understand, because you've repeated it. A lot. :-)
I'm saying you have a big EDUCATION of community issue on your hands
before releasing this feature, because if this many folks on
@internals aren't getting it, and are making senseless knee-jerk
arguments...
Then you have millions of developers, and I use the term loosely, that
will slaver over 300% performance boost and turn it on without having
the faintest idea what it does...
BREAKING the code that relies on custom error handlers, or that check
$php_errormsg
Those are the only sticking points for me.
Let me give you a real-world pragmatic scenario:
If I released "good" code with a custom error handler that gracefully
and sensibly handled errors even if somebody was dumb enough to turn
off error-reporting, and your patch breaks that, and then I get a
zillion bug reports, I'm going to be a bit unhappy, and justifiably
so.
I think there more than a few developers who fit the above scenario,
and the only way to avoid it is to be sure end users who install all
kinds of crap next to my fabulous software [tongue firmly in cheek]
and then use your new feature to shut up the bad software...
I don't want to have to deal with a thousand bug reports because your
patch encouraged the naive user to disable my error handling.
Hope that clarifies the issue I'm forseeing...
Again, I actually think you've made a good enough case for this.
I just think your "patch" needs a lot of non-code changes.
Better than average documentation.
Changes to bug-reporting page.
Comments in php.ini that clearly state that turning it "on" is BAD in
general.
.
.
.
--
Some people ask for gifts here.
I just want you to buy an Indie CD for yourself:
http://cdbaby.com/search/from/lynch
Stanislav Malyshev wrote:
Hi!
I've implemented a patch that allows to "mask" certain types of errors -
i.e. make PHP engine completely ignore them. Now even if the error is
not reported, it passes full cycle through message string creation, all
allocations on the way, etc. even though ultimately the result of it
will be thrown out. So I offer a way to make unwanted errors just
disappear.
I know I'm a nobody here but, I think this is a really important change.
I have worked in a couple of different companies where we have
specifically turned turned down the error log noise in our production
environment but kept them turned on fully (E_ALL | E_DEPRECATED) in our
dev/staging environments.
While our production environments were technically a bit quieter
log-wise, there was definitely no apparent performance benefit aside
from saving having to write to the logs. In fact, during our transition
from 4.x -> 5.x, we noticed a marked slowdown related to generation of
E_STRICT messages even when E_STRICT was disabled - which I only
realized through stepping through the source via gdb was because it was
still going through the motions of creating the messages even though
they were discarded later.
Again, I know I'm nobody here but, I really hope you find this patch
worth committing - I know I would.
Thanks for listening,
--
Carl P. Corliss
Stanislav Malyshev wrote:
Hi!
I've implemented a patch that allows to "mask" certain types of errors -
i.e. make PHP engine completely ignore them. Now even if the error is
not reported, it passes full cycle through message string creation, all
allocations on the way, etc. even though ultimately the result of it
will be thrown out. So I offer a way to make unwanted errors just
disappear.
The patch uses new directive - "error_mask", and reuses
orig_error_reporting global (which is not used anywhere in PHP source so
I wonder why it exists at all, but that's certainly convenient :) which
makes it binary-compatible and fit for 5.3. Of course, in HEAD we could
rename it.
If the value in not 0, that's the mask which would filter out all errors
that do not fit the mask (i.e. you could say in production that
everything but warnings and errors should be discarded), and if the
value is 0 then the mask is the same as error_reporting - i.e. it can be
more dynamic and account for @, etc. and all non-reported errors will be
discarded. Fatal errors will never be discarded.
Alternative approach may be to just discard all non-reported errors, but
that could be a problem with user error handlers and extension-supplied
error callbacks, so the proposed approach is more flexible as you could
control discarding and reporting separately.So, what do you think?
I think it's a good idea. Your patch does not take into account a user
error handler, however. If present, ZEND_CAN_REPORT should evaluate to
1 to match existing behavior.
For those who are worried about losing errors, I would pose a couple of
questions:
- if a tree falls in the forest and there is no one there to hear it,
do we still have to put up with the performance loss? - as long as the patch does not break any backwards compatibility
(error logging still works as it always did independent of error_mask,
user error handlers still get the same stuff), why would we care? There
is a time and place for being academic about fixing things and it is
called development, not production.
Greg
- if a tree falls in the forest and there is no one there to hear it,
do we still have to put up with the performance loss?- as long as the patch does not break any backwards compatibility
(error logging still works as it always did independent of error_mask,
user error handlers still get the same stuff), why would we care?
There
is a time and place for being academic about fixing things and it is
called development, not production.
If you have bugs in production, you sure as hell want to know about ..
and encouraging people to ignore them is a recipe for disaster.
Stats "@fopen()" example is perfect here, so lets say we do add this
feature and people simply turn of error's entirely so that they can
instead write "fopen()", they feel all good about themselves, since
they handle the error locally and get a magical speed boost
(noticeable or not) .. all the while they are ignoring all sorts of
E_NOTICES that would indicate them that they have some serious
security issues.
Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED
in production .. but there is a time for fixing E_NOTICES .. and that
time is always!
regards,
Lukas Kahwe Smith
mls@pooteeweet.org
Lukas Kahwe Smith wrote:
- if a tree falls in the forest and there is no one there to hear it,
do we still have to put up with the performance loss?- as long as the patch does not break any backwards compatibility
(error logging still works as it always did independent of error_mask,
user error handlers still get the same stuff), why would we care? There
is a time and place for being academic about fixing things and it is
called development, not production.If you have bugs in production, you sure as hell want to know about ..
and encouraging people to ignore them is a recipe for disaster.
Stats "@fopen()" example is perfect here, so lets say we do add this
feature and people simply turn of error's entirely so that they can
instead write "fopen()", they feel all good about themselves, since they
handle the error locally and get a magical speed boost (noticeable or
not) .. all the while they are ignoring all sorts of E_NOTICES that
would indicate them that they have some serious security issues.Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED
in production .. but there is a time for fixing E_NOTICES .. and that
time is always!
I don't see how this has anything to do with the patch in question. You
seem to be arguing that we shouldn't let people turn off E_NOTICE in
production. How is that related to the patch?
-Rasmus
Hi!
Stats "@fopen()" example is perfect here, so lets say we do add this
feature and people simply turn of error's entirely so that they can
instead write "fopen()", they feel all good about themselves, since they
handle the error locally and get a magical speed boost (noticeable or
not) .. all the while they are ignoring all sorts of E_NOTICES that
would indicate them that they have some serious security issues.
Actually, my point was that even if you write that code "right", only
thing you achieve is slow code. It would still not be robust, unless you
create if() and custom error handler for every imaginable error, it's
still not more secure (since all the security works on stream level and
applies to both is_readable and fopen anyway), summarily it's not better
in any way but by giving you this nice feeling you "invested into nicer
code". Which is totally imaginary since your code has virtually no
difference from the old one except being slower. And all this because of
@-phobia :)
Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED
in production .. but there is a time for fixing E_NOTICES .. and that
time is always!
If you think it is (and note it's not the law of the universe - I could
think about dozens of use cases where E_NOTICE is useless and you know
you don't want it) - go ahead and set you error mask to include
E_NOTICE, there's nothing easier. It's not like anybody is advocating
there should be only one error mask. On the contrary, I advocate
flexible approach where everybody can choose the mask that fits his
particular use case.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi,
Expanding on this.
In a typical project I always try to make my own code very clean by
letting it run on a staging environment with all error reporting
enabled for a while before living it; so keeping the error_reporting
enabled on the live server (and sending a daily log of caught errors)
would actually be helpful to me.
However, when it comes to external libraries, I hate to do clean-up
work there because it makes it harder to upgrade etc. With that in
mind, I'd like to be able to muffle any errors from those. Likelihood
is that those would cause errors in the main code anyway.
Not sure how it would work, technically, but I was thinking it'd be
nice to extend require*() and include*(). ;-)
Hi!
Stats "@fopen()" example is perfect here, so lets say we do add this
feature and people simply turn of error's entirely so that they can
instead write "fopen()", they feel all good about themselves, since they
handle the error locally and get a magical speed boost (noticeable or
not) .. all the while they are ignoring all sorts of E_NOTICES that
would indicate them that they have some serious security issues.Actually, my point was that even if you write that code "right", only
thing you achieve is slow code. It would still not be robust, unless you
create if() and custom error handler for every imaginable error, it's
still not more secure (since all the security works on stream level and
applies to both is_readable and fopen anyway), summarily it's not better
in any way but by giving you this nice feeling you "invested into nicer
code". Which is totally imaginary since your code has virtually no
difference from the old one except being slower. And all this because of
@-phobia :)Again, I am all for being able to totally ignore E_STRICT/E_DEPRECATED
in production .. but there is a time for fixing E_NOTICES .. and that
time is always!If you think it is (and note it's not the law of the universe - I could
think about dozens of use cases whereE_NOTICEis useless and you know
you don't want it) - go ahead and set you error mask to include
E_NOTICE, there's nothing easier. It's not like anybody is advocating
there should be only one error mask. On the contrary, I advocate
flexible approach where everybody can choose the mask that fits his
particular use case.Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com--
--
Tjerk
- as long as the patch does not break any backwards compatibility
(error logging still works as it always did independent of error_mask,
user error handlers still get the same stuff), why would we care?
There
is a time and place for being academic about fixing things and it is
called development, not production.
You must not have any bugs in production environment, I am very
envious. :-)
Ilia Alshanetsky wrote:
- as long as the patch does not break any backwards compatibility
(error logging still works as it always did independent of error_mask,
user error handlers still get the same stuff), why would we care? There
is a time and place for being academic about fixing things and it is
called development, not production.You must not have any bugs in production environment, I am very envious.
:-)
Ilia,
I don't see how this kind of emotional hyperbole contributes positively
to the discussion.
If I understand you right, you're arguing that people should never use
code in production that emits errors. That is certainly the goal. It's
also not always possible to do this when relying upon third-party code
which may have been written for a pre-E_STRICT or a pre-E_DEPRECATED php
version.
What I don't understand is why you think this:
<?php
error_reporting(E_WARNING|E_ERROR|E_NOTICE|E_USER_WARNING|E_USER_ERROR|E_USER_NOTICE);
class A
{
// emits an E_STRICT normally, and we waste cycles emitting an E_STRICT
that is never emitted
function A(){}
}
?>
is better than:
<?php
ini_set('error_mask',
E_WARNING|E_ERROR|E_NOTICE|E_USER_WARNING|E_USER_ERROR|E_USER_NOTICE);
class A
{
// just works (TM) as fast as it did in PHP 4
function A(){}
}
?>
IF (and a big if) no user error handler is in place, which should pass
all possible errors into the error handler.
If your fear is that third party people might do something horrendous like:
<?php
ini_set('error_mask', 0);
stupidfatalerror();
?>
That's also easily solved by making it INI_SYSTEM.
Greg
Greg,
If you enable error log you would be able to identify errors, even in
legacy code fairly quickly and address them as needed. The speed
increase, by Stas' own admission is very minimal here, I would wager
that it would be small enough that on a application generating 2-3
E_STRICTs per request you would not even be able to measure it. While
saving memory and eliminating what effectively is a NOOP is a good
idea, making it a configurable, user settable option, will simply lead
to much abuse.
If the ultimate goal here is to save memory, why not simply set the
mask to 0, which if understand Stas' original message correctly would
equate its value to whatever the error reporting is set to.
Ilia Alshanetsky wrote:
- as long as the patch does not break any backwards compatibility
(error logging still works as it always did independent of
error_mask,
user error handlers still get the same stuff), why would we care?
There
is a time and place for being academic about fixing things and it is
called development, not production.You must not have any bugs in production environment, I am very
envious.
:-)Ilia,
I don't see how this kind of emotional hyperbole contributes
positively
to the discussion.If I understand you right, you're arguing that people should never use
code in production that emits errors. That is certainly the goal.
It's
also not always possible to do this when relying upon third-party code
which may have been written for a pre-E_STRICT or a pre-E_DEPRECATED
php
version.What I don't understand is why you think this:
<?php
error_reporting(E_WARNING|E_ERROR|E_NOTICE|E_USER_WARNING|
E_USER_ERROR|E_USER_NOTICE);
class A
{
// emits anE_STRICTnormally, and we waste cycles emitting an
E_STRICT
that is never emitted
function A(){}
}
?>is better than:
<?php
ini_set('error_mask',
E_WARNING|E_ERROR|E_NOTICE|E_USER_WARNING|E_USER_ERROR|E_USER_NOTICE);
class A
{
// just works (TM) as fast as it did in PHP 4
function A(){}
}
?>IF (and a big if) no user error handler is in place, which should pass
all possible errors into the error handler.If your fear is that third party people might do something
horrendous like:<?php
ini_set('error_mask', 0);
stupidfatalerror();
?>That's also easily solved by making it INI_SYSTEM.
Greg
Hi!
If you enable error log you would be able to identify errors, even in
legacy code fairly quickly and address them as needed. The speed
increase, by Stas' own admission is very minimal here, I would wager
It's not "very minimal". It's not big overall, but it speeds up
operations affected 300-400%. Can you propose many other changes that
would speed up any set of opcodes 300% in 10 lines of patch? ;)
saving memory and eliminating what effectively is a NOOP is a good idea,
making it a configurable, user settable option, will simply lead to much
abuse.
There would be no "abuse" that is not happening today, in almost every
production install on Earth. Through the whole scenario you have
consistently ignored the fact that we talk about errors that are
ignored today, and only evidence of their general existence somewhere
in the code is the slowdown - which is not measurable anyway, since
there's no base for comparison, since there's no way to run without
the slowdown without this patch.
If the ultimate goal here is to save memory, why not simply set the mask
to 0, which if understand Stas' original message correctly would equate
its value to whatever the error reporting is set to.
You can, of course. But, as I explained, because there could be
scenarios when you (ok, not you, somebody else) would want to use
advanced tools which do not use error_reporting setting but have their
own decision mechanisms, and you may be ready to pay the memory/speed
penalty, say, for more accurate error statistics, etc. - at least
temporary or on the part of your setup.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
If you enable error log you would be able to identify errors, even
in legacy code fairly quickly and address them as needed. The speed
increase, by Stas' own admission is very minimal here, I would wagerIt's not "very minimal". It's not big overall, but it speeds up
operations affected 300-400%. Can you propose many other changes
that would speed up any set of opcodes 300% in 10 lines of patch? ;)
I have a few of those in our custom build of PHP, but none of those
would be ready for general consumption since they take away some of
PHP's conveniences. There are not many such changes, but a fair number
are possible.
saving memory and eliminating what effectively is a NOOP is a good
idea, making it a configurable, user settable option, will simply
lead to much abuse.There would be no "abuse" that is not happening today, in almost
every production install on Earth. Through the whole scenario you
have consistently ignored the fact that we talk about errors that
are ignored today, and only evidence of their general existence
somewhere in the code is the slowdown - which is not measurable
anyway, since there's no base for comparison, since there's no way
to run without the slowdown without this patch.
You had one really good example from Dan, with respect to
track_errors, which is real easy to miss. The 3rd party library maybe
using that functionality to handle certain errors and by doing a
global ingore errors, you would effectively cut down on error
information and in some cases make the code thing there is no error.
There are a few code paths I can see on google where code works
something like this:
$php_errormsg = null;
some_function();
if ($php_errormsg) {
exit($php_errormsg);
}
If you enable error log you would be able to identify errors, even
in legacy code fairly quickly and address them as needed. The speed
increase, by Stas' own admission is very minimal here, I would wagerIt's not "very minimal". It's not big overall, but it speeds up
operations affected 300-400%. Can you propose many other changes that
would speed up any set of opcodes 300% in 10 lines of patch? ;)
Considering that there shouldn't be any errors in the first place, this
is of course a moot point. Sure, 3rd party code (PEAR pcakges?) might
throw errors like crazy on PHP 5, they need to be fixed anyway. So if
the minimal overhead of the whole application is not close to
affected... I think it'd be a very bad idea to add this extra mask. Yet
another ini setting as well. For the record: I'm against adding this.
regards,
Derick
--
http://derickrethans.nl | http://ezcomponents.org | http://xdebug.org
twitter: @derickr
Hi!
Considering that there shouldn't be any errors in the first place, this
You must be kidding me. Any fopen of non-existing file, and loading of
non-perfect XML from third party produces errors. There is absolutely no
way you can write code that does anything useful that would guaranteedly
never produce errors. Moreover, any attempt to do it would make your
code 2-3 times slower, as you'd have to do most of the actions twice -
once to check if it would succeed and one to actually do it (as with
file_exists/fopen) and even then you could fail.
is of course a moot point. Sure, 3rd party code (PEAR pcakges?) might
throw errors like crazy on PHP 5, they need to be fixed anyway. So if
They won't be fixed. I am getting an impression nobody's actually
reading the comments, only writing. Once again: I'm not talking about
people that are looking for errors in their code to debug, I am talking
about setups disabling errors ON PURPOSE, because they DO NOT WANT them.
Different use case.
BTW we discussed that in Chicago meetup and agreed we want to do it. Of
course, not that agreeing on anything ever would stand in a way of
wasting another bunch of time on repeating same stuff over and over and
nobody listening and blocking another useful feature.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hello,
Hi!
Considering that there shouldn't be any errors in the first place, this
You must be kidding me. Any fopen of non-existing file, and loading of
non-perfect XML from third party produces errors. There is absolutely no way
you can write code that does anything useful that would guaranteedly never
produce errors. Moreover, any attempt to do it would make your code 2-3
times slower, as you'd have to do most of the actions twice - once to check
if it would succeed and one to actually do it (as with file_exists/fopen)
and even then you could fail.is of course a moot point. Sure, 3rd party code (PEAR pcakges?) might
throw errors like crazy on PHP 5, they need to be fixed anyway. So ifThey won't be fixed. I am getting an impression nobody's actually reading
the comments, only writing. Once again: I'm not talking about people that
are looking for errors in their code to debug, I am talking about setups
disabling errors ON PURPOSE, because they DO NOT WANT them. Different use
case.BTW we discussed that in Chicago meetup and agreed we want to do it. Of
course, not that agreeing on anything ever would stand in a way of wasting
another bunch of time on repeating same stuff over and over and nobody
listening and blocking another useful feature.
You're the one asking "What do you think?" :) People seem to think
that this feature is another invitation to bad practice in 95% of the
cases, and only useful to the last 5% of the people who know what
they're doing... (like goto?)
And no, IMO the agreements in Chicago or wherever should still pass
through the ML or something, I assume lots of people were not able to
go there, and lots of those people won't be happy with some of the
decisions.
Best,
--
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com--
--
Etienne Kneuss
http://www.colder.ch
Men never do evil so completely and cheerfully as
when they do it from a religious conviction.
-- Pascal
Hi!
You're the one asking "What do you think?" :) People seem to think
that this feature is another invitation to bad practice in 95% of the
cases, and only useful to the last 5% of the people who know what
they're doing... (like goto?)
It's useful to 100% people since these practices are being used NOW, and
this patch only targets these specific use cases when people are doing
it NOW.
And no, IMO the agreements in Chicago or wherever should still pass
through the ML or something, I assume lots of people were not able to
go there, and lots of those people won't be happy with some of the
decisions.
Yes, I know - including the very people that made that very decision.
That makes one hell of a decision taking process.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Considering that there shouldn't be any errors in the first place, this
You must be kidding me. Any fopen of non-existing file, and loading of
non-perfect XML from third party produces errors.
You can test for the first case, and the second one can be prevented by
using the libxml_errors functionality.
There is absolutely no way
you can write code that does anything useful that would guaranteedly never
produce errors.
So we should fix that instead.
They won't be fixed. I am getting an impression nobody's actually
reading the comments, only writing. Once again: I'm not talking about
people that are looking for errors in their code to debug, I am
talking about setups disabling errors ON PURPOSE, because they DO NOT
WANT them. Different use case.
And the suggestion? Yet another ini setting (some even advocate making
it INI_SYSTEM, which is obviously totally wacko)... This idea is an
ostrich method.
BTW we discussed that in Chicago meetup and agreed we want to do it.
Of course, not that agreeing on anything ever would stand in a way of
wasting another bunch of time on repeating same stuff over and over
and nobody listening and blocking another useful feature.
I don't find it useful, and obviously not everybody agreed with all the
things in Chicago either. I didn't find this back in the notes either
(http://wiki.php.net/summits/pdmnotesmay09).
regards,
Derick
Derick Rethans wrote:
Considering that there shouldn't be any errors in the first place, this
You must be kidding me. Any fopen of non-existing file, and loading of
non-perfect XML from third party produces errors.You can test for the first case, and the second one can be prevented by
using the libxml_errors functionality.
Could you show me your race-condition safe code for the first case?
-Rasmus
Derick Rethans wrote:
And the suggestion? Yet another ini setting (some even advocate making
it INI_SYSTEM, which is obviously totally wacko)... This idea is an
ostrich method.
Derick,
I'm only trying to suggest ways of fixing problems, calling them
"obviously wacko" is useless. I don't give a crap either way whether
this exact feature gets into core, but it is in the notes, although
not really in any recognizable language:
- @operators. RFC it. (Stas, need more clarification)
Let me be clear: what I am in support of is removing unnecessary
performance hit when an error is completely hidden either by
error_reporting or by @ (i.e. no track_errors, no user error handler, no
other things that would expect to see the errors). That's all that
matters to me, the suggestion on INI_SYSTEM was an example of a way to
solve a specific problem with Stas's proposed patch, probably not the
best solution, but calling it "wacko" is ridiculous.
I suggest everyone grow up and if that isn't possible, at least stop
pissing me off. I hear more than enough irrational screaming from my
month-old, there's simply no need for it on internals@ as well.
Love,
Greg
performance hit when an error is completely hidden either by
error_reporting or by @ (i.e. no track_errors, no user error handler, no
other things that would expect to see the errors). That's all that
error_get_last()...
You can't know, at the time the error should be raised, if something
later in the script wants to retrieve that error.
The only thing we can do, if we don't want to break scripts that
gracefully deal with errors, is to store the last "raw" errormsg,
errormsg arguments, caller, file, line.. and then if the message will
get used later then generate the pretty error messages "on demand".
"can do" is used very loosely there, as I don't think it can sanely be
implemented :)
The irony of this all is track_errors=off did exactly what we want,
boycott the formatting routines when nothing wants to use the results,
but after the introduction of error_get_last() (5.2.0) that ini option
is useless as the formatting routines are always executed now :)
-Hannes
Derick Rethans wrote:
And the suggestion? Yet another ini setting (some even advocate making
it INI_SYSTEM, which is obviously totally wacko)... This idea is an
ostrich method.I'm only trying to suggest ways of fixing problems, calling them
"obviously wacko" is useless. I don't give a crap either way whether
this exact feature gets into core, but it is in the notes, although
not really in any recognizable language:
- @operators. RFC it. (Stas, need more clarification)
That was some totally different discussion. We spoke about fixing @ so
that it doesn't go through the very expensive ini mechanism.
regards,
Derick
--
http://derickrethans.nl | http://ezcomponents.org | http://xdebug.org
twitter: @derickr
Hi!
I don't find it useful, and obviously not everybody agreed with all the
things in Chicago either. I didn't find this back in the notes either
(http://wiki.php.net/summits/pdmnotesmay09).
I got that you don't find it useful. I wish people could get outside
of the box and consider what others might find useful, but obviously
we're not there yet. It's not like we have millions of users with
diverse usage patterns that we would need to think of, and it's not like
we need to bother trying to find some consensual mechanism of making
decisions.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com
Hi!
<?php
ini_set('error_mask', 0);
stupidfatalerror();
?>That's also easily solved by making it INI_SYSTEM.
Note here that fatal errors can not be masked, for obvious reasons.
Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com