phpstan / phpdoc-parser

Next-gen phpDoc parser with support for intersection types and generics
MIT License
1.31k stars 62 forks source link

Avoid creating an Exception when it is not necessary. #208

Closed mad-briller closed 1 year ago

mad-briller commented 1 year ago

Exceptions capture a stack trace when they are created, not when they are thrown:

https://3v4l.org/m1b9H

Capturing this stacktrace can be fairly expensive, especially when on a hot path that is potentially very deep in the callstack such as when recursively parsing.

Moving this exception to be only be created only when necessary reduces the time to parse a fairly significantly sized phpdoc (4x the size of a carbon phpdoc) by 2-5% from my profiling with xdebug, reducing the total cost from ~52m to about ~48m on average.

ondrejmirtes commented 1 year ago

Do a fulltext search for throw $, there are even more places that could be changed. Thanks.

mad-briller commented 1 year ago

ah very nice, they don't affect the outcome of my profile too much but i don't think my test phpdoc is hitting that part of the code too much if at all

ondrejmirtes commented 1 year ago

Thank you!

levu42 commented 1 year ago

This change made the code a bit less DRY, which it didn't have to, to avoid creating the exception when not necessary. You could have created a closure that creates the exception when needed:

$exception = () => new ParserException(...);

...

throw $exception();

I'm not sure however, how much the overhead of the closure call would be here. @mad-briller if you're interested in doing it, I'd be curious of the profiling results when it's using a closure.

mad-briller commented 1 year ago

personally i'd argue that you want your exception to be created exactly where it is thrown, because of the php quirk that the stack trace is captured on creation not throw

in the old code, and also with your suggestion, when the exception is created you lose the context of where exactly it is thrown, which is useful to know when debugging stack traces passed on from users

that alone is a good reason to break the DRY principle with regards to exceptions in my opinion, ignoring any performance wins of either approach

levu42 commented 1 year ago

Good point, didn't think of that!