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:

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

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

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

Hi Dmitry,

For the behavior change that I mentioned in the other thread, with this
code:

function foo() {
static $a = -PHP_INT_MAX;
}

Which could work sometimes, and sometimes not (if in a namespace or
ZEND_COMPILE_NO_CONSTANT_SUBSTITUTION is set). I changed things so that
there is no substitution of constants (except with CT_SUBST flag, like
always) for compile time constants (ZEND_CT mode). They don't create a
FETCH_CONSTANT opcode anyway. :-)

Another thing I realized wasn't getting optimized (runtime constants), which
can be, is with:

namespace foo;
$a = ::PHP_INT_MAX; // :: for global scope

So the patch allows substitution there as well.

http://realplain.com/php/ct_const_fixes.diff
http://realplain.com/php/ct_const_fixes_5_3.diff

• 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.