Everything

php

Home » Project » Newsgroups » php.internals » [RFC][DISCUSSION] debug_backtrace alternative as an array of StackFrame objects »

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

php.internals

Michał Brzuchalski

6 years ago

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

Mike Schinkel

6 years ago

> On Jul 7, 2020, at 4:46 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

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

Michał Brzuchalski

6 years ago

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

Unnamed Person

6 years ago

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.

Michał Brzuchalski

6 years ago

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:

1) 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()`.

2) 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

Michał Brzuchalski

6 years ago

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):

tyson andre

6 years ago

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

tyson andre

6 years ago

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

Michał Brzuchalski

6 years ago

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ł

Michał Brzuchalski

6 years ago

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ł

tyson andre

6 years ago

Michał Brzuchalski

6 years ago

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ł

Nikita Popov

6 years ago

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

Michał Brzuchalski

6 years ago

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ł