[RFC][DISCUSSION] debug_backtrace alternative as an array of StackFrame objects

5 years ago by Mike Schinkel — view source — reply

unread

Hi internals,

following recently added alternative to token_get_all() in form of a
dedicated class PhpToken
I've made a similar proposal for collecting stack traces by introducing
StackFrame::getTrace()
and StackFrame which replaces array with keys and values known from
debug_backtrace().

To compete with easier adoption StackFrame implements ArrayAccess
giving identical API
to debug_backtrace().

A test script with deep recursion up to 1M frames uses 50% less memory than
debug_backtrace().

The RFC is described here https://wiki.php.net/rfc/stack-frame-class
It includes implementation of PoC and tests.

Cheers,
Michał Marcin Brzuchalski

Yes, excited to see this.

Would adding a method that allows getting a specific stack frame rather than the entire trace if you know which on you want take up less memory and/or perform better?

Something like StackFrame::getFrame(1) which would be functionally equivalent to StackFrame::getTrace()[1]?

-Mike

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

Hi Mike

śr., 8 lip 2020 o 22:20 Mike Schinkel mike@newclarity.net napisał(a):

Yes, excited to see this.

That’s good to hear.

Would adding a method that allows getting a specific stack frame rather
than the entire trace if you know which on you want take up less memory
and/or perform better?

Something like StackFrame::getFrame(1) which would be functionally
equivalent to StackFrame::getTrace()[1]?

Indeed, this makes sense.
Since I already consider adding an $offset parameter to getTrace(),
adding a dedicated static method will be trivial.

Cheers,

Michał Marcin Brzuchalski

5 years ago by Levi Morrison via internals — view source — reply

unread

On Tue, Jul 7, 2020 at 2:47 PM Michał Marcin Brzuchalski
michal.brzuchalski@gmail.com wrote:

Hi internals,

following recently added alternative to token_get_all() in form of a
dedicated class PhpToken
I've made a similar proposal for collecting stack traces by introducing
StackFrame::getTrace()
and StackFrame which replaces array with keys and values known from
debug_backtrace().

To compete with easier adoption StackFrame implements ArrayAccess
giving identical API
to debug_backtrace().

A test script with deep recursion up to 1M frames uses 50% less memory than
debug_backtrace().

The RFC is described here https://wiki.php.net/rfc/stack-frame-class
It includes implementation of PoC and tests.

Cheers,
Michał Marcin Brzuchalski

Any opposition for a parameter to limit stack depth? It's something
I'd like on the old API too.

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

Hi Levi,

śr., 8 lip 2020 o 22:25 Levi Morrison levi.morrison@datadoghq.com
napisał(a):

Any opposition for a parameter to limit stack depth? It's something
I'd like on the old API too.

I've updated the RFC with the same arguments as in debug_backtrace()
function.
I'm also considering adding a third option for int $offset = 0 in some
known cases
we want by default skip more frames at the beginning when they're known
this would
reduce copying values from skipped frames.

I've also added two additional properties and related methods:

property ?string $object_class = null and method public function getObjectClass(): ?string

They expose an object class name which is useful in case we don't wanna
populate frames with the objects.

The additional information could fulfil the
https://bugs.php.net/bug.php?id=45351 if frames as objects
would be got accepted as a replacement in Exception::getTrace().

property ?\Closure $closure = null and method public function getClosure(): ?\Closure

They expose a closure which is useful in case the frame was produced by
closure call.
This was added to keep the function property compatibility with frames
produced by debug_backtrace().

The additional information could fulfil the
https://bugs.php.net/bug.php?id=62325 if frames as objects
would be got accepted as a replacement in Exception::getTrace().

Cheers,

Michał Marcin Brzuchalski

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

Hi Internals,

Heads up, I plan to open the vote tomorrow morning.

Cheers,
Michał Marcin Brzuchalski

wt., 7 lip 2020 o 22:46 Michał Marcin Brzuchalski <
michal.brzuchalski@gmail.com> napisał(a):

Hi internals,

following recently added alternative to token_get_all() in form of a
dedicated class PhpToken
I've made a similar proposal for collecting stack traces by introducing
StackFrame::getTrace()
and StackFrame which replaces array with keys and values known from
debug_backtrace().

To compete with easier adoption StackFrame implements ArrayAccess
giving identical API
to debug_backtrace().

A test script with deep recursion up to 1M frames uses 50% less memory
than debug_backtrace().

The RFC is described here https://wiki.php.net/rfc/stack-frame-class
It includes implementation of PoC and tests.

Cheers,
Michał Marcin Brzuchalski

5 years ago by tyson andre — view source — reply

unread

Hi Michał Marcin Brzuchalski,

Heads up, I plan to open the vote tomorrow morning.

Sorry for the late comment - I'd been busy/occupied with various other things and this RFC fell off my radar,
and I'd assumed that anything I'd end up noticing when looking at the RFC
would have already been pointed out by other reviewers.
(In general, I've only reviewed or locally compiled the code of a small fraction of recent RFCs mentioned on the mailing list.)

I had some comments on the implementation in https://github.com/php/php-src/pull/5820#pullrequestreview-452045885

How much does this change elapsed time in typical use cases? This had completely skipped my mind when I saw this RFC, but I'd generally assume that stack traces aren't kept along for longer than needed to process them, and that most stack traces would be at most hundreds of frames in typical applications. So I'd consider CPU time elapsed more important than RAM.
I'd had similar issues with the tradeoffs in PhpToken (and forgot). Those issues were that the applications using the array would use less memory and be faster if I used the returned array multiple times, but less so if I only processed the array once and immediately freed it.
PHP also seems faster at fetching dimension from arrays than properties from objects.

Still, though, this might help with a bit with developer productivity (e.g. IDEs being better at inferring types of $frame->getFile() in some cases, or providing help text for the method), or in being able to check types when passing individual frames
I found a bug: The ArrayAccess magic methods are implemented, but not the real methods.
I found some memory leaks, possibly related to tracking argument arrays in the object being stored
Casting getFile() to the empty string is unexpected for me ($frame['file'] and $frame->file are null in the implementation).
Forbidding modification/deletion on the ArrayAccess magic API (e.g. $frame['trace'] = ...)
seems inconsistent when modification and deletion are allowed through the real properties,
(and throwing there may affect code written to process the array returned by Throwable->getTrace())
The typed property default was incompatible with the typed property declaration (class StackFrame { string $file = null; /*...*/} for internal stack frames)

With the feature freeze nearby, more of a heads up would have been useful.
Aside: The guidelines in https://wiki.php.net/rfc/howto have been followed,
but I think it'd be useful for those guidelines to say "consider at least one days heads up mail on the mailing list" instead of "consider one days" to avoid cases where review comments are made on short notice.
Normally, any issues found the day before voting starts would have more time to reschedule the vote to be addressed.

I don't know how amendments to the howto get proposed or decided on

Regards,

Tyson

5 years ago by tyson andre — view source — reply

unread

Hi Michał Marcin Brzuchalski,

At the time of writing, the documentation in https://wiki.php.net/rfc/stack-frame-class#stackframe_class is also inconsistent with the implementation.
public readonly string $file; is not actually read only in the implementation in the GitHub PR. It can be overwritten or deleted with $frame->file = 'newfile.php';
or unset($frame->file);.
I'd guess this is expressing the desired way that StackFrame instances would be used by PHP code,
but that seems confusing because some properties of other internal classes are actually effectively read only due to providing magic getter APIs.
(Also see my previous comment about ArrayAccess inconsistencies)
Dropping the readonly from the RFC is one possible way to clarify that.

final class StackFrame implements ArrayAccess
{
    public readonly string $file;
 
    public readonly int $line;

The first time I've compiled or looked at the implementation and RFC document in detail was this evening, out of a curiosity of how it worked
and to look into the performance impact to expect,
after seeing the vote announcement this morning.

Sorry for the late comment - I'd been busy/occupied with various other things and this RFC fell off my radar

Regards,

Tyson

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

Hi Tyson,

wt., 21 lip 2020 o 06:54 tyson andre tysonandre775@hotmail.com napisał(a):

Hi Michał Marcin Brzuchalski,

At the time of writing, the documentation in
https://wiki.php.net/rfc/stack-frame-class#stackframe_class is also
inconsistent with the implementation.
public readonly string $file; is not actually read only in the
implementation in the GitHub PR. It can be overwritten or deleted with
$frame->file = 'newfile.php';
or unset($frame->file);.
I'd guess this is expressing the desired way that StackFrame instances
would be used by PHP code,
but that seems confusing because some properties of other internal classes
are actually effectively read only due to providing magic getter APIs.
(Also see my previous comment about ArrayAccess inconsistencies)
Dropping the readonly from the RFC is one possible way to clarify that.

The readonly was dropped in the RFC to meet the implementation similar to
PhpToken where a developer is free to modify all properties.

Cheers,
Michał

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

Hi Tyson,

wt., 21 lip 2020 o 04:27 tyson andre tysonandre775@hotmail.com napisał(a):

...
I had some comments on the implementation in
https://github.com/php/php-src/pull/5820#pullrequestreview-452045885

Most of these comments can be worked upon the next days in the meantime.

How much does this change elapsed time in typical use cases? This had
completely skipped my mind when I saw this RFC, but I'd generally assume
that stack traces aren't kept along for longer than needed to process them,
and that most stack traces would be at most hundreds of frames in typical
applications. So I'd consider CPU time elapsed more important than RAM.

The current implementation works similarly as original one which instead of
creating an array creates an object of StackFrame.
In general use of resources depends on programming style, amount of
rethrows etc. Some use exceptions inappropriately and keep the objects
living for a significant time etc.

I'd had similar issues with the tradeoffs in PhpToken (and forgot).
Those issues were that the applications using the array would use less
memory and be faster if I used the returned array multiple times, but less
so if I only processed the array once and immediately freed it.
PHP also seems faster at fetching dimension from arrays than properties
from objects.

Still, though, this might help with a bit with developer productivity
(e.g. IDEs being better at inferring types of $frame->getFile() in some
cases, or providing help text for the method), or in being able to check
types when passing individual frames

I found a bug: The ArrayAccess magic methods are implemented, but not
the real methods.

This can be fixed, PR provides an initial implementation which can be
improved in the next days.

I found some memory leaks, possibly related to tracking argument arrays
in the object being stored

Casting getFile() to the empty string is unexpected for me
($frame['file'] and $frame->file are null in the implementation).

Forbidding modification/deletion on the ArrayAccess magic API (e.g.
$frame['trace'] = ...)
seems inconsistent when modification and deletion are allowed through
the real properties,
(and throwing there may affect code written to process the array
returned by Throwable->getTrace())

This can be adjusted to meet 99% compatibility with current array frames,
the only limitation I would see here would be the inability to create
properties dynamically.

The typed property default was incompatible with the typed property
declaration (class StackFrame { string $file = null; /*...*/} for
internal stack frames)

Can you give an example of an internal stack frame?

Cheers,
Michał

5 years ago by tyson andre — view source — reply

unread

Can you give an example of an internal stack frame?

A stack frame that was created by a PHP internal function instead of by userland code, i.e. one without a file in the frame.
Related to https://github.com/php/php-src/pull/5820#pullrequestreview-452080212

php > array_map(fn() => $GLOBALS['x'] = StackFrame::getTrace()[0], [2]);
php > var_export($x);
StackFrame::__set_state(array(
   'file' => NULL,
   'line' => NULL,
   'function' => '{closure}',
   'class' => NULL,
   'type' => NULL,
   'object' => NULL,
   'object_class' => NULL,
   'closure' => NULL,
   'args' => 
  array (
    0 => 2,
  ),
))

Regards,

Tyson

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

wt., 21 lip 2020 o 07:07 tyson andre tysonandre775@hotmail.com napisał(a):

Can you give an example of an internal stack frame?

A stack frame that was created by a PHP internal function instead of by
userland code, i.e. one without a file in the frame.
Related to
https://github.com/php/php-src/pull/5820#pullrequestreview-452080212
php > array_map(fn() => $GLOBALS['x'] = StackFrame::getTrace()[0], [2]);
php > var_export($x);
StackFrame::__set_state(array(
   'file' => NULL,
   'line' => NULL,
   'function' => '{closure}',
   'class' => NULL,
   'type' => NULL,
   'object' => NULL,
   'object_class' => NULL,
   'closure' => NULL,
   'args' =>
  array (
    0 => 2,
  ),
))

The RFC has been adjusted regarding the nullability of file and line
properties and related getter methods.

Is there anything else stopping it from getting up to the vote?

Cheers,
Michał

5 years ago by Nikita Popov — view source — reply

unread

On Tue, Jul 21, 2020 at 7:14 AM Michał Marcin Brzuchalski <
michal.brzuchalski@gmail.com> wrote:

wt., 21 lip 2020 o 07:07 tyson andre tysonandre775@hotmail.com
napisał(a):
Can you give an example of an internal stack frame?

A stack frame that was created by a PHP internal function instead of by
userland code, i.e. one without a file in the frame.
Related to
https://github.com/php/php-src/pull/5820#pullrequestreview-452080212
php > array_map(fn() => $GLOBALS['x'] = StackFrame::getTrace()[0], [2]);
php > var_export($x);
StackFrame::__set_state(array(
   'file' => NULL,
   'line' => NULL,
   'function' => '{closure}',
   'class' => NULL,
   'type' => NULL,
   'object' => NULL,
   'object_class' => NULL,
   'closure' => NULL,
   'args' =>
  array (
    0 => 2,
  ),
))
The RFC has been adjusted regarding the nullability of file and line
properties and related getter methods.

Is there anything else stopping it from getting up to the vote?

Cheers,
Michał

Why does StackFrame define both public typed properties and getter
methods? One or the other would make sense to me, but not both.

Nikita

5 years ago by michal.brzuchalski@gmail.com — view source — reply

unread

Hi Nikita,

wt., 21 lip 2020 o 10:41 Nikita Popov nikita.ppv@gmail.com napisał(a):

Why does StackFrame define both public typed properties and getter
methods? One or the other would make sense to me, but not both.

TBH I didn't know what should be the preference, but since a StackFrame
class is final providing methods doesn't bring any value in the API.

I've updated the RFC and have removed all the methods and their references.
Will update the PR with appropriate changes and resolve pending comments in
next days.

Cheers,
Michał