Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39277
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 212.25.124.163 as permitted sender)
Message-ID: <4888C03E.8040305@zend.com>
Date: Thu, 24 Jul 2008 21:47:42 +0400
User-Agent: Thunderbird 2.0.0.16 (Windows/20080708)
MIME-Version: 1.0
To: Matt Wilmas <php_lists@realplain.com>
CC: internals@lists.php.net, Andi Gutmans <andi@zend.com>, 
 Stanislav Malyshev <stas@zend.com>
References: <00ea01c8a160$2edd8160$0201a8c0@pc1> <016c01c8eccd$e28cfac0$0201a8c0@pc1> <488835D0.1040005@zend.com> <00e501c8ed77$3498cf20$0201a8c0@pc1> <48889901.8000008@zend.com> <027701c8eda4$1ef01890$0201a8c0@pc1>
In-Reply-To: <027701c8eda4$1ef01890$0201a8c0@pc1>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] No runtime fetching of built-in global constants
From: dmitry@zend.com (Dmitry Stogov)

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
>>
>>
>