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: > > 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: > >>> 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 > > > ---------------------------------------------------------------------------- ---- > 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 > > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
