Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:39274
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain realplain.com from 209.235.152.153 cause and error)
Message-ID: <024f01c8eda1$ee9949c0$0201a8c0@pc1>
To: <internals@lists.php.net>, "Dmitry Stogov" <dmitry@zend.com>
Cc: "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> <4888710B.5020406@zend.com>
Date: Thu, 24 Jul 2008 10:28:31 -0500
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [PATCH] No runtime fetching of built-in global constants
From: php_lists@realplain.com ("Matt Wilmas")

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