# [PATCH] No runtime fetching of built-in global constants - Thread: https://externals.io/message/37134 - Original archive: https://news-web.php.net/php.internals/37134 - Started by Matt Wilmas on 2008-04-18T14:26:22+00:00 - Messages: 39 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/37134.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #37134 — Matt Wilmas — 2008-04-18T14:26:22+00:00 URL: https://externals.io/message/37134#37134 Hi all, I changed things so that the many "built-in" constants (CONST_PERSISTENT ones) will be replaced at compile-time, saving the FETCH_CONSTANT opcode, if these changes are usable. This was added for TRUE/FALSE/NULL 2 years ago, but seems like it can be done for "lots" of others too. Since the change 2 years ago, other constants have been getting looked up also, but just discarded. And if a constant wasn't found, its name was lowercased and looked up again (for case-insensitive TRUE/FALSE/NULL). Lowercasing has been removed, since case-insensitive constants can't be done (guess an exception was made for TRUE/FALSE/NULL :-)), and TRUE/FALSE/NULL get flagged in the scanner (not reserved words, which Marcus did briefly a few years ago), skipping a hash lookup. BTW, to get this compile-time optimization in a namespace, it needs to be prefixed (::CONSTANT). I also removed an unnecessary memcmp() in zend_get_constant() -- old code that was needed a long time ago, it appears. http://realplain.com/php/const_ct_optimization.diff http://realplain.com/php/const_ct_optimization_5_3.diff Thoughts? Thanks, Matt --- ## #39227 — Matt Wilmas — 2008-07-23T14:10:37+00:00 URL: https://externals.io/message/37134#39227 · In reply to: #37134 Hi all, Never heard anything about this optimization after sending it 3 months ago (should've sent a follow-up sooner)... Is this something that can be done? Dmitry? Details in original message. Patch is unchanged, I just updated them for the current file versions. http://realplain.com/php/const_ct_optimization.diff http://realplain.com/php/const_ct_optimization_5_3.diff Thanks, Matt ----- Original Message ----- From: "Matt Wilmas" Sent: Friday, April 18, 2008 > Hi all, > > I changed things so that the many "built-in" constants (CONST_PERSISTENT > ones) will be replaced at compile-time, saving the FETCH_CONSTANT opcode, if > these changes are usable. This was added for TRUE/FALSE/NULL 2 years ago, > but seems like it can be done for "lots" of others too. > > Since the change 2 years ago, other constants have been getting looked up > also, but just discarded. And if a constant wasn't found, its name was > lowercased and looked up again (for case-insensitive TRUE/FALSE/NULL). > Lowercasing has been removed, since case-insensitive constants can't be done > (guess an exception was made for TRUE/FALSE/NULL :-)), and TRUE/FALSE/NULL > get flagged in the scanner (not reserved words, which Marcus did briefly a > few years ago), skipping a hash lookup. BTW, to get this compile-time > optimization in a namespace, it needs to be prefixed (::CONSTANT). > > I also removed an unnecessary memcmp() in zend_get_constant() -- old code > that was needed a long time ago, it appears. > > http://realplain.com/php/const_ct_optimization.diff > http://realplain.com/php/const_ct_optimization_5_3.diff > > Thoughts? > > > Thanks, > Matt --- ## #39256 — Dmitry Stogov — 2008-07-24T07:57:04+00:00 URL: https://externals.io/message/37134#39256 · In reply to: #39227 Hi Matt, Sorry if I missed it. Does this patch make any performance difference? I assume it saves on hash lookup during compilation and its really insignificant time. However it add new scanner rules which may slowdown the whole scanner. For now I don't see a big reason, but may be I didn't see something. Do you have any other "postponed" patches? I remember something about string optimizations and long multiply. Thanks. Dmitry. Matt Wilmas wrote: > Hi all, > > Never heard anything about this optimization after sending it 3 months ago > (should've sent a follow-up sooner)... > > Is this something that can be done? Dmitry? Details in original message. > Patch is unchanged, I just updated them for the current file versions. > > http://realplain.com/php/const_ct_optimization.diff > http://realplain.com/php/const_ct_optimization_5_3.diff > > > Thanks, > Matt > > > ----- Original Message ----- > From: "Matt Wilmas" > Sent: Friday, April 18, 2008 > > > […] > if > > […] > done > > […] --- ## #39262 — Matt Wilmas — 2008-07-24T10:22:40+00:00 URL: https://externals.io/message/37134#39262 · In reply to: #39256 Hi Dmitry, ----- Original Message ----- From: "Dmitry Stogov" Sent: Thursday, July 24, 2008 > Hi Matt, > > Sorry if I missed it. No problem. :-) > Does this patch make any performance difference? > > I assume it saves on hash lookup during compilation and its really > insignificant time. However it add new scanner rules which may slowdown > the whole scanner. > For now I don't see a big reason, but may be I didn't see something. Yes, I tried to explain the differences in the original message (below). In runtime, FETCH_CONSTANT is saved for the "built-in, global constants" (CONST_PERSISTENT). During compilation, no hash lookup is needed for TRUE/FALSE/NULL since they are caught by the scanner, as you saw. The compile-time hash lookups were added when you eliminated runtime fetching for TRUE/FALSE/NULL a couple years ago, which has since been looking up the other built-in constants too and discarding them, so I just use them. :-) Also, right now if the constant isn't found (zend_get_ct_const()), there's lowercasing and a second lookup -- only for catching case-insensitive TRUE/FALSE/NULL! One thing I was wondering about is if this would cause a problem for opcode caches if they need to know it's really a "constant constant" and not a "literal constant." If so, can probably have a compiler_options flag to disable my compile-time substitution, and the opcode cache can do what it wants (substitution with own optimizer, etc.). > Do you have any other "postponed" patches? > I remember something about string optimizations and long multiply. Yeah. :-) The multiply long one [1] is a pretty small thing that probably should be reviewed for a decision (MM's safe_address() function especially), though it does speed up mul_function() (* operator) on more platforms. The "string changes/optimizations" patch [2] is mostly fine, I think. Just wondering if it's OK to remove the INIT_STRING opcode. BTW, it has changes that make the scanner simpler/smaller if you're concerned about the constants patch adding a few rules. ;-D [1] http://marc.info/?l=php-internals&m=121630449331340&w=2 [2] http://marc.info/?l=php-internals&m=121569400218314&w=2 > Thanks. Dmitry. > Thanks, Matt > Matt Wilmas wrote: > > […] ago > > […] message. > > […] (CONST_PERSISTENT > > […] opcode, > > […] ago, > > […] up > > […] TRUE/FALSE/NULL > > […] briefly a > > […] code > > […] --- ## #39266 — Dmitry Stogov — 2008-07-24T12:09:47+00:00 URL: https://externals.io/message/37134#39266 · In reply to: #39262 According to constants patch, it definitely will break PHP encoders and may be opcode caches, but as you mentioned the compiler_option will solve the issue. In this case we probable may substitute any constants (not only persistent). Anyway I don't see a big reason for special handling of TRUE/FALSE/NULL in scanner. Also it'll break "function true(){}" and may be something else (I'm not sure if it's good or bad :). Multiple long looks fine, but on which systems did you test it? I'll try to take a deeper look into string optimization patch today or tomorrow. Thanks. Dmitry. Matt Wilmas wrote: > Hi Dmitry, > > ----- Original Message ----- > From: "Dmitry Stogov" > Sent: Thursday, July 24, 2008 > > > […] > > No problem. :-) > > > […] > > Yes, I tried to explain the differences in the original message (below). In > runtime, FETCH_CONSTANT is saved for the "built-in, global constants" > (CONST_PERSISTENT). During compilation, no hash lookup is needed for > TRUE/FALSE/NULL since they are caught by the scanner, as you saw. The > compile-time hash lookups were added when you eliminated runtime fetching > for TRUE/FALSE/NULL a couple years ago, which has since been looking up the > other built-in constants too and discarding them, so I just use them. :-) > Also, right now if the constant isn't found (zend_get_ct_const()), there's > lowercasing and a second lookup -- only for catching case-insensitive > TRUE/FALSE/NULL! > > One thing I was wondering about is if this would cause a problem for opcode > caches if they need to know it's really a "constant constant" and not a > "literal constant." If so, can probably have a compiler_options flag to > disable my compile-time substitution, and the opcode cache can do what it > wants (substitution with own optimizer, etc.). > > > […] > > Yeah. :-) The multiply long one [1] is a pretty small thing that probably > should be reviewed for a decision (MM's safe_address() function especially), > though it does speed up mul_function() (* operator) on more platforms. > > The "string changes/optimizations" patch [2] is mostly fine, I think. Just > wondering if it's OK to remove the INIT_STRING opcode. BTW, it has changes > that make the scanner simpler/smaller if you're concerned about the > constants patch adding a few rules. ;-D > > [1] http://marc.info/?l=php-internals&m=121630449331340&w=2 > [2] http://marc.info/?l=php-internals&m=121569400218314&w=2 > > > […] > > Thanks, > Matt > > > > […] > ago > > […] > message. > > […] > (CONST_PERSISTENT > > […] > opcode, > > […] > ago, > > […] > up > > […] > TRUE/FALSE/NULL > > […] > briefly a > > […] > code > > […] --- ## #39270 — Dmitry Stogov — 2008-07-24T15:00:17+00:00 URL: https://externals.io/message/37134#39270 · In reply to: #39262 I would propose the attached patch for this optimization. Opcode caches and encoders will have to disable this optimization with ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION Any objections? Thanks. Dmitry. Matt Wilmas wrote: > Hi Dmitry, > > ----- Original Message ----- > From: "Dmitry Stogov" > Sent: Thursday, July 24, 2008 > > > […] > > No problem. :-) > > > […] > > Yes, I tried to explain the differences in the original message (below). In > runtime, FETCH_CONSTANT is saved for the "built-in, global constants" > (CONST_PERSISTENT). During compilation, no hash lookup is needed for > TRUE/FALSE/NULL since they are caught by the scanner, as you saw. The > compile-time hash lookups were added when you eliminated runtime fetching > for TRUE/FALSE/NULL a couple years ago, which has since been looking up the > other built-in constants too and discarding them, so I just use them. :-) > Also, right now if the constant isn't found (zend_get_ct_const()), there's > lowercasing and a second lookup -- only for catching case-insensitive > TRUE/FALSE/NULL! > > One thing I was wondering about is if this would cause a problem for opcode > caches if they need to know it's really a "constant constant" and not a > "literal constant." If so, can probably have a compiler_options flag to > disable my compile-time substitution, and the opcode cache can do what it > wants (substitution with own optimizer, etc.). > > > […] > > Yeah. :-) The multiply long one [1] is a pretty small thing that probably > should be reviewed for a decision (MM's safe_address() function especially), > though it does speed up mul_function() (* operator) on more platforms. > > The "string changes/optimizations" patch [2] is mostly fine, I think. Just > wondering if it's OK to remove the INIT_STRING opcode. BTW, it has changes > that make the scanner simpler/smaller if you're concerned about the > constants patch adding a few rules. ;-D > > [1] http://marc.info/?l=php-internals&m=121630449331340&w=2 > [2] http://marc.info/?l=php-internals&m=121569400218314&w=2 > > > […] > > Thanks, > Matt > > > > […] > ago > > […] > message. > > […] > (CONST_PERSISTENT > > […] > opcode, > > […] > ago, > > […] > up > > […] > TRUE/FALSE/NULL > > […] > briefly a > > […] > code > > […] --- ## #39274 — Matt Wilmas — 2008-07-24T15:28:31+00:00 URL: https://externals.io/message/37134#39274 · In reply to: #39266 Hi Dmitry, ----- Original Message ----- From: "Dmitry Stogov" Sent: Thursday, July 24, 2008 > According to constants patch, it definitely will break PHP encoders and > may be opcode caches, but as you mentioned the compiler_option will > solve the issue. In this case we probable may substitute any constants > (not only persistent). I just figured the persistent ones were about all that could be done (and safe to substitute). Almost every built-in constant is persistent -- except SID (session id) that I know of, which can be redefined during runtime, I believe. And user constants wouldn't necessarily exist yet, unless they were `define()`'d before an include, etc... > Anyway I don't see a big reason for special > handling of TRUE/FALSE/NULL in scanner. Also it'll break "function > true(){}" and may be something else (I'm not sure if it's good or bad :). No, it won't break function true() {} or anything, I kept that in mind (though I think it's bad) :-), and it still returns `T_STRING` (too bad it has to duplicate string still when it's actually a constant). Marcus added T_TRUE, etc. 4 years ago and then reverted it because of what you mention, I assume (reserved word, though I wouldn't be against it!). See v1.112 and 1.115 of zend_language_scanner.l. (OK, I just now saw your next message with the different patch... will reply to that also.) My thinking with the special handling of TRUE/FALSE/NULL in the scanner was to save the lowercasing and second lookup if they aren't written in lowercase (and it happens for all other constants that aren't found, e.g. user ones). Just seems to waste more time than needed. > Multiple long looks fine, but on which systems did you test it? > > I'll try to take a deeper look into string optimization patch today or > tomorrow. > > Thanks. Dmitry. Thanks, Matt > Matt Wilmas wrote: > > […] In > > […] fetching > > […] the > > […] :-) > > […] there's > > […] opcode > > […] it > > […] probably > > […] especially), > > […] Just > > […] changes > > […] looked > > […] was > > […] TRUE/FALSE/NULL). > > […] be > > […] compile-time > > […] --- ## #39276 — Matt Wilmas — 2008-07-24T15:44:11+00:00 URL: https://externals.io/message/37134#39276 · In reply to: #39270 Hi again Dmitry, Well, that should get the main runtime optimization job done just as well. :-) I was just trying for more compile-time improvement also (it was definitely measurable with fake tests), especially for those not using an opcode cache, with no lookup needed for the 3 basic constants, and only one without lowercasing for others. One other thing it looks like with your patch, to be careful of, is wrongly substituting SID (session id) like I mentioned in the previous message, or other case-insensitive constants that could match at compile time, to later be defined as case sensitive, which should have priority. Know what I mean? Thanks, Matt ----- Original Message ----- From: "Dmitry Stogov" Sent: Thursday, July 24, 2008 > I would propose the attached patch for this optimization. > > Opcode caches and encoders will have to disable this optimization with > ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION > > Any objections? > > Thanks. Dmitry. > > Matt Wilmas wrote: > > […] In > > […] fetching > > […] the > > […] :-) > > […] there's > > […] opcode > > […] it > > […] probably > > […] especially), > > […] Just > > […] changes > > […] looked > > […] was > > […] TRUE/FALSE/NULL). > > […] be > > […] compile-time > > […] > ---------------------------------------------------------------------------- ---- > Index: Zend/zend_compile.c > =================================================================== > RCS file: /repository/ZendEngine2/zend_compile.c,v > retrieving revision 1.647.2.27.2.41.2.74 > diff -u -p -d -r1.647.2.27.2.41.2.74 zend_compile.c > --- Zend/zend_compile.c 24 Jul 2008 11:47:49 -0000 1.647.2.27.2.41.2.74 > +++ Zend/zend_compile.c 24 Jul 2008 14:40:12 -0000 > @@ -3804,6 +3804,12 @@ static zend_constant* zend_get_ct_const( > if (c->flags & CONST_CT_SUBST) { > return c; > } > + if (!CG(current_namespace) && > + !(CG(compiler_options) & ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION) && > + Z_TYPE(c->value) != IS_CONSTANT && > + Z_TYPE(c->value) != IS_CONSTANT_ARRAY) { > + return c; > + } > return NULL; > } > /* }}} */ > @@ -5169,12 +5175,14 @@ void zend_do_use(znode *ns_name, znode * > void zend_do_declare_constant(znode *name, znode *value TSRMLS_DC) /* {{{ */ > { > zend_op *opline; > + zend_constant *c; > > if(Z_TYPE(value->u.constant) == IS_CONSTANT_ARRAY) { > zend_error(E_COMPILE_ERROR, "Arrays are not allowed as constants"); > } > > - if (zend_get_ct_const(&name->u.constant TSRMLS_CC)) { > + c = zend_get_ct_const(&name->u.constant TSRMLS_CC); > + if (c && (c->flags & CONST_CT_SUBST)) { > zend_error(E_COMPILE_ERROR, "Cannot redeclare constant '%s'", Z_STRVAL(name->u.constant)); > } > > Index: Zend/zend_compile.h > =================================================================== > RCS file: /repository/ZendEngine2/zend_compile.h,v > retrieving revision 1.316.2.8.2.12.2.27 > diff -u -p -d -r1.316.2.8.2.12.2.27 zend_compile.h > --- Zend/zend_compile.h 24 Jul 2008 11:47:49 -0000 1.316.2.8.2.12.2.27 > +++ Zend/zend_compile.h 24 Jul 2008 14:40:13 -0000 > @@ -762,6 +762,9 @@ END_EXTERN_C() > /* generate ZEND_DECLARE_INHERITED_CLASS_DELAYED opcode to delay early binding */ > #define ZEND_COMPILE_DELAYED_BINDING (1<<4) > > +/* disable constant substitution at compile-time */ > +#define ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION (1<<5) > + > /* The default value for CG(compiler_options) */ > #define ZEND_COMPILE_DEFAULT ZEND_COMPILE_HANDLE_OP_ARRAY --- ## #39277 — Dmitry Stogov — 2008-07-24T17:47:42+00:00 URL: https://externals.io/message/37134#39277 · In reply to: #39276 Thank you for noticing SID issue. So it seems like we able to substitute only persistent constants. Thanks. Dmitry. Matt Wilmas wrote: > Hi again Dmitry, > > Well, that should get the main runtime optimization job done just as well. > :-) I was just trying for more compile-time improvement also (it was > definitely measurable with fake tests), especially for those not using an > opcode cache, with no lookup needed for the 3 basic constants, and only one > without lowercasing for others. > > One other thing it looks like with your patch, to be careful of, is wrongly > substituting SID (session id) like I mentioned in the previous message, or > other case-insensitive constants that could match at compile time, to later > be defined as case sensitive, which should have priority. Know what I mean? > > > Thanks, > Matt > > > ----- Original Message ----- > From: "Dmitry Stogov" > Sent: Thursday, July 24, 2008 > > > […] > In > > […] > fetching > > […] > the > > […] > :-) > > […] > there's > > […] > opcode > > […] > it > > […] > probably > > […] > especially), > > […] > Just > > […] > changes > > […] > looked > > […] > was > > […] > TRUE/FALSE/NULL). > > […] > be > > […] > compile-time > > […] > > > ---------------------------------------------------------------------------- > ---- > > > > […] > {{{ */ > > […] > Z_STRVAL(name->u.constant)); > > […] > binding */ > > […] --- ## #39325 — Matt Wilmas — 2008-07-25T16:34:47+00:00 URL: https://externals.io/message/37134#39325 · In reply to: #39270 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. Checking Z_TYPE(c->value) != IS_CONSTANT[_ARRAY] isn't needed with the persistent check now, is it? 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... - Matt ----- Original Message ----- From: "Dmitry Stogov" Sent: Thursday, July 24, 2008 > I would propose the attached patch for this optimization. > > Opcode caches and encoders will have to disable this optimization with > ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION > > Any objections? > > Thanks. Dmitry. > ---------------------------------------------------------------------------- ---- > Index: Zend/zend_compile.c > =================================================================== > RCS file: /repository/ZendEngine2/zend_compile.c,v > retrieving revision 1.647.2.27.2.41.2.74 > diff -u -p -d -r1.647.2.27.2.41.2.74 zend_compile.c > --- Zend/zend_compile.c 24 Jul 2008 11:47:49 -0000 1.647.2.27.2.41.2.74 > +++ Zend/zend_compile.c 24 Jul 2008 14:40:12 -0000 > @@ -3804,6 +3804,12 @@ static zend_constant* zend_get_ct_const( > if (c->flags & CONST_CT_SUBST) { > return c; > } > + if (!CG(current_namespace) && > + !(CG(compiler_options) & ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION) && > + Z_TYPE(c->value) != IS_CONSTANT && > + Z_TYPE(c->value) != IS_CONSTANT_ARRAY) { > + return c; > + } > return NULL; > } > /* }}} */ > @@ -5169,12 +5175,14 @@ void zend_do_use(znode *ns_name, znode * > void zend_do_declare_constant(znode *name, znode *value TSRMLS_DC) /* {{{ */ > { > zend_op *opline; > + zend_constant *c; > > if(Z_TYPE(value->u.constant) == IS_CONSTANT_ARRAY) { > zend_error(E_COMPILE_ERROR, "Arrays are not allowed as constants"); > } > > - if (zend_get_ct_const(&name->u.constant TSRMLS_CC)) { > + c = zend_get_ct_const(&name->u.constant TSRMLS_CC); > + if (c && (c->flags & CONST_CT_SUBST)) { > zend_error(E_COMPILE_ERROR, "Cannot redeclare constant '%s'", Z_STRVAL(name->u.constant)); > } > > Index: Zend/zend_compile.h > =================================================================== > RCS file: /repository/ZendEngine2/zend_compile.h,v > retrieving revision 1.316.2.8.2.12.2.27 > diff -u -p -d -r1.316.2.8.2.12.2.27 zend_compile.h > --- Zend/zend_compile.h 24 Jul 2008 11:47:49 -0000 1.316.2.8.2.12.2.27 > +++ Zend/zend_compile.h 24 Jul 2008 14:40:13 -0000 > @@ -762,6 +762,9 @@ END_EXTERN_C() > /* generate ZEND_DECLARE_INHERITED_CLASS_DELAYED opcode to delay early binding */ > #define ZEND_COMPILE_DELAYED_BINDING (1<<4) > > +/* disable constant substitution at compile-time */ > +#define ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION (1<<5) > + > /* The default value for CG(compiler_options) */ > #define ZEND_COMPILE_DEFAULT ZEND_COMPILE_HANDLE_OP_ARRAY --- ## #39369 — Dmitry Stogov — 2008-07-27T10:56:36+00:00 URL: https://externals.io/message/37134#39369 · In reply to: #39325 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. 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] 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. > 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. > 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. Thanks. Dmitry. > > - Matt > > > ----- Original Message ----- > From: "Dmitry Stogov" > Sent: Thursday, July 24, 2008 > > > […] > > > ---------------------------------------------------------------------------- > ---- > > > > […] > {{{ */ > > […] > Z_STRVAL(name->u.constant)); > > […] > binding */ > > […] --- ## #39373 — Matt Wilmas — 2008-07-27T13:35:11+00:00 URL: https://externals.io/message/37134#39373 · In reply to: #39369 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: > > […] attached > > […] > > 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. > > […] > > I think you are right, but it's better to have this checks, because > nobody prohibit to create array constants in extensions. OK. :-) > > […] > > 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 --- ## #39380 — Arnaud Le Blanc — 2008-07-27T14:22:50+00:00 URL: https://externals.io/message/37134#39380 · In reply to: #39373 Hi, On Sunday 27 July 2008 15:35:11 Matt Wilmas wrote: > > 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... The shebang line is used by the operating system to know which interpreter to use to execute the file. In most cases this does not cause any problem to the interpretor as lines starting with # are comments in many languages, but in PHP it's not a comment as long as it's not enclosed with , so PHP must check if there is a shebang line and skip it. > > 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. :-) Actually the CGI SAPI opens the file, seeks after the shebang line, and then passes the opened file descriptor to the scanner. I see two solutions for that: Make the scanner completely bypass the shebang line if cgi.check_shebang_line==1 Or let the executor decide wether to output the shebang or not by enclosing it in a "SHEBANG" opcode for example. Regards, Arnaud --- ## #39392 — Nuno Lopes — 2008-07-28T04:04:18+00:00 URL: https://externals.io/message/37134#39392 · In reply to: #39369 I'm not Matt, but I'll try to answer :) Actually the new re2c scanner already handles the shebang thing, so I think you can safely remove the explicit support for it in CGI. We had to had that because CLI doesn't explicitly support the shebang line. Nuno P.S.: now it makes sense why we never found the code in the flex scanner that handled the shebang line :P > 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. > > 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. --- ## #39393 — Stanislav Malyshev — 2008-07-28T05:42:13+00:00 URL: https://externals.io/message/37134#39393 · In reply to: #39369 Hi! > 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. But for most cases you'd just do cgi.check_shebang_line=0 in php.ini and that should solve the problem, not? -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #39394 — Dmitry Stogov — 2008-07-28T06:55:51+00:00 URL: https://externals.io/message/37134#39394 · In reply to: #39373 Hi Matt, It would be nice if scanner check for "#!" at start of file and skip the whole first line if found. Nothing else. Of curse it won't save syscall itself. Now shebang line is checked (in CGI SAPI code) before call to compiler and in case of scanner modification this code might be removed. So with opcode caches, which override the compiler routine, this syscall will be saved. I'll review your constant notes/patches later today. Thanks. Dmitry. Matt Wilmas wrote: > Hi Dmitry, > > ----- Original Message ----- > From: "Dmitry Stogov" > Sent: Sunday, July 27, 2008 > > > […] > > 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. :-) > > > […] > > Yes, more reply below... > > > […] > attached > > […] > > 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. > > > […] > > OK. :-) > > > […] > > 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! > :-) > > > […] > > - Matt --- ## #39396 — Dmitry Stogov — 2008-07-28T07:14:46+00:00 URL: https://externals.io/message/37134#39396 · In reply to: #39393 For now it solves the check code (read/seek) but not the open syscall. Dmitry Stanislav Malyshev wrote: > Hi! > > > […] > > But for most cases you'd just do cgi.check_shebang_line=0 in php.ini and > that should solve the problem, not? --- ## #39397 — Stanislav Malyshev — 2008-07-28T07:24:06+00:00 URL: https://externals.io/message/37134#39397 · In reply to: #39396 Hi! > For now it solves the check code (read/seek) but not the open syscall. Hm... this looks wrong. Why one needs to open if scanner is perfectly capable of opening file by itself? -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #39398 — Dmitry Stogov — 2008-07-28T07:26:06+00:00 URL: https://externals.io/message/37134#39398 · In reply to: #39397 It is the thing I like to do. Dmitry. Stanislav Malyshev wrote: > Hi! > > > […] > > Hm... this looks wrong. Why one needs to open if scanner is perfectly > capable of opening file by itself? --- ## #39399 — Dmitry Stogov — 2008-07-28T07:47:57+00:00 URL: https://externals.io/message/37134#39399 · In reply to: #39392 It's great. So it's nothing to do in the scanner. I can just change the CGI code. Thanks. Dmitry. Nuno Lopes wrote: > I'm not Matt, but I'll try to answer :) > > Actually the new re2c scanner already handles the shebang thing, so I > think you can safely remove the explicit support for it in CGI. We had > to had that because CLI doesn't explicitly support the shebang line. > > Nuno > > P.S.: now it makes sense why we never found the code in the flex > scanner that handled the shebang line :P > > > > > […] --- ## #39411 — Stanislav Malyshev — 2008-07-28T16:57:49+00:00 URL: https://externals.io/message/37134#39411 · In reply to: #39399 Hi! > It's great. So it's nothing to do in the scanner. > I can just change the CGI code. Note that if there's shebang enabled, there could be options in the command line. And then CGI/CLI would have to process them, I guess. -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #39412 — Dmitry Stogov — 2008-07-28T17:44:15+00:00 URL: https://externals.io/message/37134#39412 · In reply to: #39411 I tested scanner and now it really handless shebang lines itself without CGI. So the cgi.enable_shebang_line and corresponding code can be safely removed. However we still need to handle "not found"/"access denied" cases and respond with 404/403. Thanks. Dmitry. Stanislav Malyshev wrote: > Hi! > > > […] > > Note that if there's shebang enabled, there could be options in the > command line. And then CGI/CLI would have to process them, I guess. --- ## #39413 — Stanislav Malyshev — 2008-07-28T17:59:23+00:00 URL: https://externals.io/message/37134#39413 · In reply to: #39412 Hi! > I tested scanner and now it really handless shebang lines itself without > CGI. So the cgi.enable_shebang_line and corresponding code can be safely > removed. However we still need to handle "not found"/"access denied" > cases and respond with 404/403. How scanner could handle command-line options? -- Stanislav Malyshev, Zend Software Architect stas@zend.com http://www.zend.com/ (408)253-8829 MSN: stas@zend.com --- ## #39414 — Dmitry Stogov — 2008-07-28T18:39:46+00:00 URL: https://externals.io/message/37134#39414 · In reply to: #39413 It does't have to do it. It just checks for shebang line always, so we don't have to do it in SAPI code. Dmitry. Stanislav Malyshev wrote: > Hi! > > > […] > > How scanner could handle command-line options? --- ## #39416 — Lukas Kahwe Smith — 2008-07-28T19:39:45+00:00 URL: https://externals.io/message/37134#39416 · In reply to: #39412 > I tested scanner and now it really handless shebang lines itself > without CGI. So the cgi.enable_shebang_line and corresponding code > can be safely removed. However we still need to handle "not > found"/"access denied" cases and respond with 404/403. @Dmitry/Scott: So how are you guys doing in terms of applying Matt's patches? I guess its time to focus on the upcoming release on Thursday, so lets get a bit more conservative in the patches that are applied. That being said Johannes and my plan defined that after alpha1 we would not have any more feature engine additions and only very specific self contained extensions would be allowed to see feature additions (if they are nodded off by the RMs). Bug fixes are another story of course .. Is this ok with everybody? Any stumbling blocks? regards, Lukas Kahwe Smith mls@pooteeweet.org --- Next page: https://externals.io/message/37134.md?page=2