Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39266
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: <4888710B.5020406@zend.com>
Date: Thu, 24 Jul 2008 16:09:47 +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>
In-Reply-To: <00e501c8ed77$3498cf20$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)

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
>