Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Parser: inline unnecessary method calls #45

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

Parser: inline unnecessary method calls #45

wants to merge 1 commit into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Mar 27, 2021 •
edited

within the Parser class before this PR the code was concentrated on reducing duplicated stuff.
this lead to rather deep function stacks, because simple calls to e.g. Parser::run() require some levels of indirection, before the actual implemenation does its work.

the php engine needs to manage a lot of function call stacks because of this method stacking

grafik

before this change

$ /c/xampp7.4.16/php/php vendor/bin/phpbench run benchmarks --report=aggregate
PHPBench @git_tag@ running benchmarks...
with configuration file: C:\xampp7.3\htdocs\redaxo\redaxo\src\addons\parsica/phpbench.json
with PHP version 7.4.16, xdebug ❌, opcache ❌

\JSONBench

    bench_json_encode.......................I2 - Mo12.588μs (±0.752%)
    bench_Parsica_JSON......................I2 - Mo20,475.211μs (±1.444%)
    bench_basemax_jpophp....................I2 - Mo1,464.014μs (±3.597%)

\ManyBench

    bench_takeWhile.........................I9 - Mo361.308μs (±7.581%)
    bench_manySatisfy.......................I9 - Mo838.244μs (±7.176%)
    bench_manyChar..........................I9 - Mo869.862μs (±3.366%)
    bench_oldManySatisfy....................I9 - Mo2,318.895μs (±1.972%)
    bench_oldManyChar.......................I9 - Mo2,378.941μs (±2.378%)

⅀T: 133,480.200μs μSD/r 70.901μs μRSD/r: 3.533%

Subjects: 8, Assertions: 0, Failures: 0, Errors: 0
suite: 1346297a94977420650f90fa9959b3d62b5f7f84, date: 2021-03-27, stime: 20:33:28
+-----------+----------------------+-----+------+-----+------------+--------------+--------------+--------------+--------------+-----------+--------+-----------+
| benchmark | subject              | set | revs | its | mem_peak   | best         | mean         | mode         | worst        | stdev     | rstdev | diff      |
+-----------+----------------------+-----+------+-----+------------+--------------+--------------+--------------+--------------+-----------+--------+-----------+
| JSONBench | bench_json_encode    | 0   | 5    | 3   | 1,704,816b | 12.400μs     | 12.533μs     | 12.588μs     | 12.600μs     | 0.094μs   | 0.75%  | 1.00x     |
| JSONBench | bench_Parsica_JSON   | 0   | 5    | 3   | 2,200,456b | 19,904.600μs | 20,313.667μs | 20,475.211μs | 20,578.000μs | 293.346μs | 1.44%  | 1,620.77x |
| JSONBench | bench_basemax_jpophp | 0   | 5    | 3   | 1,910,288b | 1,431.600μs  | 1,488.133μs  | 1,464.014μs  | 1,560.000μs  | 53.529μs  | 3.60%  | 118.73x   |
| ManyBench | bench_takeWhile      | 0   | 10   | 10  | 1,857,576b | 305.200μs    | 346.090μs    | 361.308μs    | 382.900μs    | 26.238μs  | 7.58%  | 27.61x    |
| ManyBench | bench_manySatisfy    | 0   | 10   | 10  | 1,894,280b | 807.600μs    | 859.850μs    | 838.244μs    | 1,031.300μs  | 61.706μs  | 7.18%  | 68.61x    |
| ManyBench | bench_manyChar       | 0   | 10   | 10  | 1,895,024b | 831.500μs    | 877.430μs    | 869.862μs    | 926.200μs    | 29.537μs  | 3.37%  | 70.01x    |
| ManyBench | bench_oldManySatisfy | 0   | 10   | 10  | 2,991,512b | 2,274.700μs  | 2,339.960μs  | 2,318.895μs  | 2,420.400μs  | 46.154μs  | 1.97%  | 186.70x   |
| ManyBench | bench_oldManyChar    | 0   | 10   | 10  | 2,992,264b | 2,282.100μs  | 2,380.390μs  | 2,378.941μs  | 2,495.400μs  | 56.602μs  | 2.38%  | 189.92x   |
+-----------+----------------------+-----+------+-----+------------+--------------+--------------+--------------+--------------+-----------+--------+-----------+

after this change

$ /c/xampp7.4.16/php/php vendor/bin/phpbench run benchmarks --report=aggregate
PHPBench @git_tag@ running benchmarks...
with configuration file: C:\xampp7.3\htdocs\redaxo\redaxo\src\addons\parsica/phpbench.json
with PHP version 7.4.16, xdebug ❌, opcache ❌

\JSONBench

    bench_json_encode.......................I2 - Mo12.654μs (±8.86%)
    bench_Parsica_JSON......................I2 - Mo19,501.333μs (±1.932%)
    bench_basemax_jpophp....................I2 - Mo1,356.023μs (±1.051%)

\ManyBench

    bench_takeWhile.........................I9 - Mo306.684μs (±4.176%)
    bench_manySatisfy.......................I9 - Mo811.244μs (±2.847%)
    bench_manyChar..........................I9 - Mo867.358μs (±3.101%)
    bench_oldManySatisfy....................I9 - Mo2,358.584μs (±2.892%)
    bench_oldManyChar.......................I9 - Mo2,475.324μs (±2.93%)

⅀T: 132,379.000μs μSD/r 75.234μs μRSD/r: 3.473%

Subjects: 8, Assertions: 0, Failures: 0, Errors: 0
suite: 134629723379e3063d8989d486b7142f739efdae, date: 2021-03-27, stime: 20:36:10
+-----------+----------------------+-----+------+-----+------------+--------------+--------------+--------------+--------------+-----------+--------+-----------+
| benchmark | subject              | set | revs | its | mem_peak   | best         | mean         | mode         | worst        | stdev     | rstdev | diff      |
+-----------+----------------------+-----+------+-----+------------+--------------+--------------+--------------+--------------+-----------+--------+-----------+
| JSONBench | bench_json_encode    | 0   | 5    | 3   | 1,704,816b | 12.400μs     | 13.333μs     | 12.654μs     | 15.000μs     | 1.181μs   | 8.86%  | 1.00x     |
| JSONBench | bench_Parsica_JSON   | 0   | 5    | 3   | 2,200,776b | 19,233.600μs | 19,650.533μs | 19,501.333μs | 20,151.800μs | 379.567μs | 1.93%  | 1,473.79x |
| JSONBench | bench_basemax_jpophp | 0   | 5    | 3   | 1,910,288b | 1,352.600μs  | 1,364.200μs  | 1,356.023μs  | 1,384.400μs  | 14.336μs  | 1.05%  | 102.32x   |
| ManyBench | bench_takeWhile      | 0   | 10   | 10  | 1,857,896b | 299.000μs    | 312.710μs    | 306.684μs    | 339.900μs    | 13.059μs  | 4.18%  | 23.45x    |
| ManyBench | bench_manySatisfy    | 0   | 10   | 10  | 1,892,600b | 788.800μs    | 818.740μs    | 811.244μs    | 878.400μs    | 23.312μs  | 2.85%  | 61.41x    |
| ManyBench | bench_manyChar       | 0   | 10   | 10  | 1,893,344b | 825.100μs    | 862.580μs    | 867.358μs    | 913.600μs    | 26.748μs  | 3.10%  | 64.69x    |
| ManyBench | bench_oldManySatisfy | 0   | 10   | 10  | 2,990,328b | 2,340.400μs  | 2,408.430μs  | 2,358.584μs  | 2,538.100μs  | 69.640μs  | 2.89%  | 180.63x   |
| ManyBench | bench_oldManyChar    | 0   | 10   | 10  | 2,991,080b | 2,429.100μs  | 2,527.020μs  | 2,475.324μs  | 2,645.800μs  | 74.031μs  | 2.93%  | 189.53x   |
+-----------+----------------------+-----+------+-----+------------+--------------+--------------+--------------+--------------+-----------+--------+-----------+

staabm
staabm commented Mar 27, 2021
View reviewed changes
src/Parser.php
@@ -283,7 +283,11 @@ public function and(Parser $other): Parser
*/
public function tryString(string $input): ParseResult
{
return $this->try(new StringStream($input));
Copy link
Contributor Author

@staabm staabm Mar 27, 2021 •
edited

Choose a reason for hiding this comment

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

e.g. instead of calling $this->try(), which calls $this->run() which then invokes the actual parserFunction this simple change saves 3 levels of function call stacking on every tryString invocation, by "copying" the implementation details directly in here.

@staabm staabm changed the title Parser: inline unnecessary function calls Parser: inline unnecessary method calls Mar 28, 2021
@staabm
Copy link
Contributor Author

staabm commented Apr 30, 2021

Any feedback on this change @turanct ?

@staabm staabm mentioned this pull request Apr 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@staabm