Skip to content

Improve performance of instantiating exceptions/errors #18442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nielsdos
Copy link
Member

The class structure is fixed, so it makes no sense to go through all the logic of looking up property info etc.
This patch introduces a local function zend_obj_prop_num_checked() to help with that.

For this benchmark:

for ($i = 0; $i < 1000000; $i++)
   new Error;

On an i7-4790:

Benchmark 1: ./sapi/cli/php  x.php
  Time (mean ± σ):     141.6 ms ±   9.3 ms    [User: 138.7 ms, System: 2.0 ms]
  Range (min … max):   135.4 ms … 177.7 ms    20 runs

Benchmark 2: ../RELx64_old/sapi/cli/php x.php
  Time (mean ± σ):     214.1 ms ±   7.0 ms    [User: 207.6 ms, System: 5.0 ms]
  Range (min … max):   206.6 ms … 230.9 ms    13 runs

Summary
  ./sapi/cli/php  x.php ran
    1.51 ± 0.11 times faster than ../RELx64_old/sapi/cli/php x.php

For this benchmark:

for ($i = 0; $i < 1000000; $i++)
    new Exception("message", 0, null);

On an i7-4790:

Benchmark 1: ./sapi/cli/php  x.php
  Time (mean ± σ):     184.3 ms ±   9.5 ms    [User: 181.2 ms, System: 1.8 ms]
  Range (min … max):   173.8 ms … 205.1 ms    15 runs

Benchmark 2: ../RELx64_old/sapi/cli/php x.php
  Time (mean ± σ):     253.7 ms ±   7.0 ms    [User: 247.6 ms, System: 4.6 ms]
  Range (min … max):   245.7 ms … 263.7 ms    11 runs

Summary
  ./sapi/cli/php  x.php ran
    1.38 ± 0.08 times faster than ../RELx64_old/sapi/cli/php x.php

For this benchmark:

for ($i = 0; $i < 1000000; $i++)
    new ErrorException("message", 0, 0, "xyz", 0, null);

On an i7-4790:

Benchmark 1: ./sapi/cli/php  x.php
  Time (mean ± σ):     223.6 ms ±   7.7 ms    [User: 220.1 ms, System: 2.4 ms]
  Range (min … max):   216.9 ms … 242.5 ms    12 runs

Benchmark 2: ../RELx64_old/sapi/cli/php x.php
  Time (mean ± σ):     343.5 ms ±   8.1 ms    [User: 337.1 ms, System: 4.6 ms]
  Range (min … max):   337.3 ms … 362.8 ms    10 runs

Summary
  ./sapi/cli/php  x.php ran
    1.54 ± 0.06 times faster than ../RELx64_old/sapi/cli/php x.php

The class structure is fixed, so it makes no sense to go through all the
logic of looking up property info etc.
This patch introduces a local function `zend_obj_prop_num_checked()` to
help with that.

For this benchmark:
```php
for ($i = 0; $i < 1000000; $i++)
   new Error;
```

On an i7-4790:
```
Benchmark 1: ./sapi/cli/php  x.php
  Time (mean ± σ):     141.6 ms ±   9.3 ms    [User: 138.7 ms, System: 2.0 ms]
  Range (min … max):   135.4 ms … 177.7 ms    20 runs

Benchmark 2: ../RELx64_old/sapi/cli/php x.php
  Time (mean ± σ):     214.1 ms ±   7.0 ms    [User: 207.6 ms, System: 5.0 ms]
  Range (min … max):   206.6 ms … 230.9 ms    13 runs

Summary
  ./sapi/cli/php  x.php ran
    1.51 ± 0.11 times faster than ../RELx64_old/sapi/cli/php x.php
```

For this benchmark:
```php
for ($i = 0; $i < 1000000; $i++)
    new Exception("message", 0, null);
```

On an i7-4790:
```
Benchmark 1: ./sapi/cli/php  x.php
  Time (mean ± σ):     184.3 ms ±   9.5 ms    [User: 181.2 ms, System: 1.8 ms]
  Range (min … max):   173.8 ms … 205.1 ms    15 runs

Benchmark 2: ../RELx64_old/sapi/cli/php x.php
  Time (mean ± σ):     253.7 ms ±   7.0 ms    [User: 247.6 ms, System: 4.6 ms]
  Range (min … max):   245.7 ms … 263.7 ms    11 runs

Summary
  ./sapi/cli/php  x.php ran
    1.38 ± 0.08 times faster than ../RELx64_old/sapi/cli/php x.php
```

For this benchmark:
```php
for ($i = 0; $i < 1000000; $i++)
    new ErrorException("message", 0, 0, "xyz", 0, null);
```

On an i7-4790:
```
Benchmark 1: ./sapi/cli/php  x.php
  Time (mean ± σ):     223.6 ms ±   7.7 ms    [User: 220.1 ms, System: 2.4 ms]
  Range (min … max):   216.9 ms … 242.5 ms    12 runs

Benchmark 2: ../RELx64_old/sapi/cli/php x.php
  Time (mean ± σ):     343.5 ms ±   8.1 ms    [User: 337.1 ms, System: 4.6 ms]
  Range (min … max):   337.3 ms … 362.8 ms    10 runs

Summary
  ./sapi/cli/php  x.php ran
    1.54 ± 0.06 times faster than ../RELx64_old/sapi/cli/php x.php
```
@nielsdos nielsdos marked this pull request as ready for review April 27, 2025 12:38
Copy link
Contributor

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't measured recently but I know when people say that exceptions are expensive, they also generally factor in walking the call stack.

My employer actually has a blog article about this, written by my coworker @realFlowControl: https://www.datadoghq.com/blog/php-exception-profiling/. The blog is a sort of advertisement for the exception profiler, of course, but it has a meaningful benchmark in it that you could run that's roughly 85 call frames deep, which matched a real performance issue one of our customers had.

I'd be very curious to see your benchmarking numbers with this patch. Of course, I support any maintainable performance improvement, so in that sense, I definitely approve! I just want to understand on a deeper stack, how much this helps. And maybe adjust the "up to 50% faster" wording to be a bit more realistic :)

@nielsdos
Copy link
Member Author

I haven't measured recently but I know when people say that exceptions are expensive, they also generally factor in walking the call stack.

Sure, but I'm also convinced that can be improved performance-wise 🙂

(...) but it has a meaningful benchmark in it that you could run that's roughly 85 call frames deep, which matched a real performance issue one of our customers had.

I might take a look at using that as a benchmark to improve backtrace construction, if that's something I would work on in the future.

And maybe adjust the "up to 50% faster" wording to be a bit more realistic :)

Sure

Comment on lines +335 to +337
zval *tmp = zend_obj_prop_num_checked(Z_OBJ_P(object), 6, ZSTR_KNOWN(ZEND_STR_PREVIOUS));
zval_ptr_dtor(tmp);
ZVAL_COPY(tmp, previous);
Copy link
Member

@bwoebi bwoebi Apr 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You sadly have to do the garbage-move dance as you cannot rely on the __construct being called before any object would have been assigned to these values - at least those properties which can contain objects. I.e. previous in Exception and ErrorException.

zend_class_entry *old_scope = EG(fake_scope);
EG(fake_scope) = i_get_exception_base(object);
const zend_property_info *prop_info = zend_get_property_info(object->ce, str, true);
ZEND_ASSERT(OBJ_PROP_TO_NUM(prop_info->offset) == prop_num);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can depend on this since #17870. These properties are not final, so a child class may add hooks to them. A simple solution would be to check for ce->num_hooked_props == 0 and use a fallback to zend_update_property_ex() otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine for the private properties though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, that would be fine. Not all properties are here though.

@arnaud-lb
Copy link
Member

arnaud-lb commented Apr 29, 2025

This is great. The same technique could possibly be used for other internal classes that have standard properties.

Just some thoughts, not meant to block this PR:

We could use stub files to generate offset macros/constants, and expose zend_obj_prop_num_checked(), or generate accessors for each prop.

Since Exceptions are almost user land classes (they have no custom handlers except clone_obj), I'm wondering how it would perform if we actually declared these classes in user land (c.f. #18204). Edit: it's slower.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants