[PATCH] No runtime fetching of built-in global constants

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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

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

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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

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

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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

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

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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

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

18 years ago by Matt Wilmas — view source — reply

unread

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

18 years ago by Arnaud Le Blanc — view source — reply

unread

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 <?php ?>, 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

18 years ago by Dmitry Stogov — view source — reply

unread

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

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

18 years ago by Nuno Lopes — view source — reply

unread

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.

18 years ago by Dmitry Stogov — view source — reply

unread

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

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.

18 years ago by Stanislav Malyshev — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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!

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.

18 years ago by Stanislav Malyshev — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

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!

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?

18 years ago by Lukas Kahwe Smith — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

Hi Lukas,

I don't expect any significant changes from my side any more.
All the patches, which I liked to look into, already committed.

Of course I have several bugs assigned to me and I'll try to fix them,
but it shouldn't affect 5.3.0 release process.

Especially this FastCGI change is just an idea. I don't have a final
solution yet and of course 5.3.0 shouldn't wait for it.

I'm fain with your release plan.

Thanks. Dmitry.

Lukas Kahwe Smith wrote:

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

18 years ago by Stanislav Malyshev — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

For now it solves the check code (read/seek) but not the open syscall.

Dmitry

Stanislav Malyshev wrote:

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?

18 years ago by Stanislav Malyshev — view source — reply

unread

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

18 years ago by Dmitry Stogov — view source — reply

unread

It is the thing I like to do.

Dmitry.

Stanislav Malyshev wrote:

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?

18 years ago by Matt Wilmas — view source — reply

unread

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.

18 years ago by Dmitry Stogov — view source — reply

unread

Thanks Matt. I committed near the same patch.
It's not so optimal, but little bit more clear.

Thanks. Dmitry.

Matt Wilmas wrote:

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.

18 years ago by Matt Wilmas — view source — reply

unread

Hi Dmitry,

Do you know that with your changes, no substitution will happen in a
namespace even when using :: prefix? :-/ (That's what I would do when I
know it's global, for optimization.) Or is that what you meant by "not so
optimal?"

Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Thursday, July 31, 2008

Thanks Matt. I committed near the same patch.
It's not so optimal, but little bit more clear.

Thanks. Dmitry.

Matt Wilmas wrote:

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

18 years ago by Dmitry Stogov — view source — reply

unread

Hi Matt,

Now I know. :)
Anyway, I don't think this optimization will work often.

Send me the patch after Alpha1 release.

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Do you know that with your changes, no substitution will happen in a
namespace even when using :: prefix? :-/ (That's what I would do when I
know it's global, for optimization.) Or is that what you meant by "not so
optimal?"

Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Thursday, July 31, 2008

Thanks Matt. I committed near the same patch.
It's not so optimal, but little bit more clear.

Thanks. Dmitry.

Matt Wilmas wrote:

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

18 years ago by Matt Wilmas — view source — reply

unread

Hi Dmitry,

Well, it's been awhile since Alpha 1 :-), so I wanted to finally resend this
before Alpha 2! I agree that the additional optimization probably wouldn't
happen often, as there won't be that much namespace usage right away, I
assume. But I think it makes sense to handle :: prefix constants, since
they're known to have global scope, and I can see the future [online]
optimization tip: "When using namespaces, use :: for global constants to get
compile-time substitution." ;-)

The code is the same as before, just updated the patch. It doesn't really
seem less clear, to me, than the current code. In zend_do_fetch_constant(),
the "check_namespace" variable may be better named something like
"from_namespace" according to my changes, to be more clear, but I left that
out to make the patch as simple as possible.

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

Thanks,
Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Thursday, July 31, 2008

Hi Matt,

Now I know. :)
Anyway, I don't think this optimization will work often.

Send me the patch after Alpha1 release.

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Do you know that with your changes, no substitution will happen in a
namespace even when using :: prefix? :-/ (That's what I would do when I
know it's global, for optimization.) Or is that what you meant by "not
so
optimal?"

Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Thursday, July 31, 2008

Thanks Matt. I committed near the same patch.
It's not so optimal, but little bit more clear.

Thanks. Dmitry.

Matt Wilmas wrote:

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

18 years ago by Lukas Kahwe Smith — view source — reply

unread

Well, it's been awhile since Alpha 1 :-), so I wanted to finally
resend this
before Alpha 2! I agree that the additional optimization probably
wouldn't
happen often, as there won't be that much namespace usage right
away, I
assume. But I think it makes sense to handle :: prefix constants,
since
they're known to have global scope, and I can see the future [online]
optimization tip: "When using namespaces, use :: for global
constants to get
compile-time substitution." ;-)

Dmitry .. if you find time to commit this tomorrow its on, otherwise
this one has to stay out of 5.3.0. Speaking of which, tomorrow really
is the hard deadline for feature additions. This includes additions to
self contained extensions and of course anything engine related.

From Friday on the focus will be on producing a rock solid alpha2,
with only clear bug and build fixes. With a beta1 hopefully to follow
within 2-3 weeks. We will then see how soon we can move to RC stage. A
release in October seems optimistic at this point, but not totally
undoable. But things might have to slip into early November.

regards,
Lukas Kahwe Smith
mls@pooteeweet.org

18 years ago by Dmitry Stogov — view source — reply

unread

Hi Matt,

I updated your patch a little bit to make it more clear (from my point
of view).
Please take a look.

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Well, it's been awhile since Alpha 1 :-), so I wanted to finally resend this
before Alpha 2! I agree that the additional optimization probably wouldn't
happen often, as there won't be that much namespace usage right away, I
assume. But I think it makes sense to handle :: prefix constants, since
they're known to have global scope, and I can see the future [online]
optimization tip: "When using namespaces, use :: for global constants to get
compile-time substitution." ;-)

The code is the same as before, just updated the patch. It doesn't really
seem less clear, to me, than the current code. In zend_do_fetch_constant(),
the "check_namespace" variable may be better named something like
"from_namespace" according to my changes, to be more clear, but I left that
out to make the patch as simple as possible.

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

Thanks,
Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Thursday, July 31, 2008

Hi Matt,

Now I know. :)
Anyway, I don't think this optimization will work often.

Send me the patch after Alpha1 release.

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Do you know that with your changes, no substitution will happen in a
namespace even when using :: prefix? :-/ (That's what I would do when I
know it's global, for optimization.) Or is that what you meant by "not
so
optimal?"

Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Thursday, July 31, 2008

Thanks Matt. I committed near the same patch.
It's not so optimal, but little bit more clear.

Thanks. Dmitry.

Matt Wilmas wrote:

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

18 years ago by Matt Wilmas — view source — reply

unread

Hi Dmitry,

Yeah, that looks good too, and should work the same way. :-)

Thanks,
Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Friday, August 29, 2008

Hi Matt,

I updated your patch a little bit to make it more clear (from my point
of view).
Please take a look.

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Well, it's been awhile since Alpha 1 :-), so I wanted to finally resend
this
before Alpha 2! I agree that the additional optimization probably
wouldn't
happen often, as there won't be that much namespace usage right away, I
assume. But I think it makes sense to handle :: prefix constants, since
they're known to have global scope, and I can see the future [online]
optimization tip: "When using namespaces, use :: for global constants to
get
compile-time substitution." ;-)

The code is the same as before, just updated the patch. It doesn't
really
seem less clear, to me, than the current code. In
zend_do_fetch_constant(),
the "check_namespace" variable may be better named something like
"from_namespace" according to my changes, to be more clear, but I left
that
out to make the patch as simple as possible.

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

Thanks,
Matt

18 years ago by Dmitry Stogov — view source — reply

unread

Ok, I'm going to commit it.
Could you remember why we disabled constants substitution for ZEND_CT?

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Yeah, that looks good too, and should work the same way. :-)

Thanks,
Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Friday, August 29, 2008

Hi Matt,

I updated your patch a little bit to make it more clear (from my point
of view).
Please take a look.

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Well, it's been awhile since Alpha 1 :-), so I wanted to finally resend
this
before Alpha 2! I agree that the additional optimization probably
wouldn't
happen often, as there won't be that much namespace usage right away, I
assume. But I think it makes sense to handle :: prefix constants, since
they're known to have global scope, and I can see the future [online]
optimization tip: "When using namespaces, use :: for global constants to
get
compile-time substitution." ;-)

The code is the same as before, just updated the patch. It doesn't
really
seem less clear, to me, than the current code. In
zend_do_fetch_constant(),
the "check_namespace" variable may be better named something like
"from_namespace" according to my changes, to be more clear, but I left
that
out to make the patch as simple as possible.

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

Thanks,
Matt

18 years ago by Matt Wilmas — view source — reply

unread

Hi Dmitry,

----- Original Message -----
From: "Dmitry Stogov"
Sent: Friday, August 29, 2008

Ok, I'm going to commit it.
Could you remember why we disabled constants substitution for ZEND_CT?

Yeah, it's to make sure something like -CONST in ZEND_CT context doesn't
work sometimes, sometimes not. My previous message (first part):
http://marc.info/?l=php-internals&m=121750618525882&w=2

Thanks. Dmitry.

Matt Wilmas wrote:

Hi Dmitry,

Yeah, that looks good too, and should work the same way. :-)

Thanks,
Matt

----- Original Message -----
From: "Dmitry Stogov"
Sent: Friday, August 29, 2008

Hi Matt,

I updated your patch a little bit to make it more clear (from my point
of view).
Please take a look.

Thanks. Dmitry.

18 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

Yeah, it's to make sure something like -CONST in ZEND_CT context doesn't
work sometimes, sometimes not. My previous message (first part):
http://marc.info/?l=php-internals&m=121750618525882&w=2

There's also a thing that now code like:
$var = 3/0;
(of course, it could be more complex - both 3 and 0 could be constants)
does not compile, even though it may never in fact run, so there might
be some BC issues.

Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com

18 years ago by Matt Wilmas — view source — reply

unread

Hi Stas,

----- Original Message -----
From: "Stanislav Malyshev"
Sent: Friday, August 29, 2008

Hi!

Yeah, it's to make sure something like -CONST in ZEND_CT context
doesn't
work sometimes, sometimes not. My previous message (first part):
http://marc.info/?l=php-internals&m=121750618525882&w=2

There's also a thing that now code like:
$var = 3/0;
(of course, it could be more complex - both 3 and 0 could be constants)
does not compile, even though it may never in fact run, so there might
be some BC issues.

I'm not sure what you mean, does not compile? Nothing has been changed that
should affect any code like your example...

Matt

18 years ago by Stanislav Malyshev — view source — reply

unread

Hi!

I'm not sure what you mean, does not compile? Nothing has been changed that
should affect any code like your example...

Oh, I think I was confusing two constant patches - I was thinking about
the "constant evaluation" patch, not "constant fetching" patch. Sorry.

Stanislav Malyshev, Zend Software Architect
stas@zend.com http://www.zend.com/
(408)253-8829 MSN: stas@zend.com

[PATCH] No runtime fetching of built-in global constants View as Markdown

Index: Zend/zend_compile.c

Index: Zend/zend_compile.h

Index: Zend/zend_compile.c

Index: Zend/zend_compile.h

Index: Zend/zend_compile.c

Index: Zend/zend_compile.h

Index: Zend/zend_compile.c

Index: Zend/zend_compile.h

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.

How scanner could handle command-line options?

But for most cases you'd just do cgi.check_shebang_line=0 in php.ini and that should solve the problem, not?

There's also a thing that now code like: $var = 3/0; (of course, it could be more complex - both 3 and 0 could be constants) does not compile, even though it may never in fact run, so there might be some BC issues.

Oh, I think I was confusing two constant patches - I was thinking about the "constant evaluation" patch, not "constant fetching" patch. Sorry.

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.

But for most cases you'd just do cgi.check_shebang_line=0 in php.ini and
that should solve the problem, not?

There's also a thing that now code like:
$var = 3/0;
(of course, it could be more complex - both 3 and 0 could be constants)
does not compile, even though it may never in fact run, so there might
be some BC issues.

Oh, I think I was confusing two constant patches - I was thinking about
the "constant evaluation" patch, not "constant fetching" patch. Sorry.