Hi Dmitry,
----- Original Message -----
From: "Dmitry Stogov"
Sent: Sunday, July 27, 2008
Hi Matt,
At first as you are a scanner expert, I would like you to look into
another optimization idea.
Probably for historical reason PHP supports shebang lines
(#! /usr/bin/php) on top of php files. Especially to handle them PHP
(CGI/FastCGI/CLI) opens file and check for it. So even with opcode
caches FastCGI PHP does open syscall for the requested script, however
with opcode caches it's absolutely useless.
In case PHP scanner will handle shebang lines itself, we will able to
save this syscall.
Sorry, but now I'm the one who's confused here, since I have no idea what
I'm supposed to look into exactly. :-/ I know about shebang lines, and I
know there's a check in the scanner now to skip over it (must have been
somewhere else with flex...). PHP itself doesn't "use" the shebang, does
it? I don't have much knowledge of *nix system stuff, but I thought the
shebang is for the OS to use when asked to run an executable script...
So I don't understand what could be changed in the scanner to save the open
syscall -- since if the scanner is called, the file has already been opened,
right? Again, sorry, but I'll need more explanation. :-)
I never had time and enough flex/re2c knowledge to implement this idea
myself. May be you'll able to look into the problem. In case you find a
simple solution we will able to do it in php-5.3.
Most PHP hosters and large sites use FastCGI with opcode caches (it is
also the primary way for MS Windows users), so this optimization is
really important.
[see below]
Yes, more reply below...
Matt Wilmas wrote:
Hi Dmitry,
I saw that you commited this patch, with the addition of only replacing
persistent constants (just mentioning for others on the list). The
attached
patches have a few tweaks:
The main thing I noticed is that if "something" creates a persistent,
case-INsensitive constant (any of those in "standard" PHP, besides
TRUE/FALSE/NULL?), it could still wrongly be substituted. My change
eliminates that possibility.
After yesterday's subbotnik I'm so stupid so cannot understand simple
tings. :)
Could you point me into the exact part of the patch.
Sure, I'll explain more. If something (extension, etc...) creates a
persistent constant "foo" and it does NOT have the CONST_CS flag, and
capital "FOO" or whatever is used in a script (and a case-sensitive version
doesn't exist, of course), it will be lowercased, as you know, and "foo"
will be found, but that can't be used since a case-sensitive version may be
defined later (TRUE/FALSE/NULL are exceptions, but OK to do since they have
CONST_CT_SUBST). Current zend_get_ct_const() behavior after finding
case-insensitive "foo":
if ((c->flags & CONST_CS) && ...) {
efree(lookup_name);
return NULL; /* Doesn't fail for "foo" since CONST_CS isn't
set... */
.....
if (c->flags & CONST_CT_SUBST) {
return c; /* This doesn't happen since CONST_CT_SUBST isn't set... */
}
if ((c->flags & CONST_PERSISTENT) &&
...) {
return c; /* This DOES happen since CONST_PERSISTENT is set */
}
The last part is the problem, of course. Substitution should only be done
for ones that have:
CONST_CT_SUBST
or
CONST_PERSISTENT and whose case matches that used in the script (CONST_CS or
lowercase is used, e.g. "foo" in script is OK, not "FOO" or "Foo")
(BTW, that also implies that CT_SUBST is only useful for case-insensitive
constants (like T/F/N) and if something sets it with (CS | PERSISTENT), it
doesn't really do anything -- except not allow it to be used in "const ..."
declarations. :-P Though that could be desired, by something, and/or to
also have substitution even in a namespace.)
With my change, after finding case-insensitive "foo":
if ((c->flags & CONST_CT_SUBST) && !(c->flags & CONST_CS)) {
efree(lookup_name);
return c; /* Happens for TRUE/FALSE/NULL */
}
}
efree(lookup_name);
return NULL; /* Happens for "foo" before persistent check */
Hope that wasn't TOO verbose. :-O I just swapped the logic (succeed vs
fail) of that top part so the else { } block could be removed.
Checking Z_TYPE(c->value) != IS_CONSTANT[_ARRAY] isn't needed with the
persistent check now, is it?
I think you are right, but it's better to have this checks, because
nobody prohibit to create array constants in extensions.
OK. :-)
From orginal patch (zend_constants.c part), the memcmp(...) != 0 isn't
needed as it will always be true. If it wasn't, the *first* hash lookup
wouldn't have failed. :-) Like I said in the original message, it's old
code from a long time ago, but was never removed...
I'll check it with more clear head.
I looked through the CVS logs/versions for zend_constants.c again, and the
memcmp() was needed before v1.40 (6 years ago). That's when all constants
were lowercased first, and then case was checked. 1.40 added case-sensitive
lookup first: memcmp() no longer necessary, as I said. And this old code
was then copied to zend_get_ct_const(), etc. That code area shouldn't get
hit very often or anything, but might as well remove since it's redundant!
:-)
Thanks. Dmitry.
- Matt
