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
