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

Reuse Name instances to improve performance #106

Merged
merged 2 commits into from
Apr 7, 2023
Merged

Reuse Name instances to improve performance #106

merged 2 commits into from
Apr 7, 2023

Conversation

tobyzerner
Copy link
Contributor

@tobyzerner tobyzerner commented Apr 5, 2023 •
edited
Loading

I have a project with a lot of components and dom_id or dom_class being called hundreds of times on each page load.

This PR optimizes the performance of these functions by reusing Name instances such that the name for a particular class only ever has to be calculated once.

The calculation (in the Tonysm\TurboLaravel\Models\Naming\Name::build method) is actually somewhat expensive when done in large quantities – mostly as a result of the use of the config helper in removeRootNamespaces (resolving from the container each time adds up), and the call to Str::plural.

Ideally the public properties of the Name class would be made readonly so these instances are immutable, however would require dropping PHP 8.0 support.

@tonysm tonysm self-requested a review April 6, 2023 16:53
tonysm
tonysm reviewed Apr 7, 2023
View reviewed changes
src/Models/Naming/Name.php Outdated Show resolved Hide resolved
tonysm
tonysm approved these changes Apr 7, 2023
View reviewed changes
Copy link
Collaborator

@tonysm tonysm left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@tonysm tonysm merged commit 8a27373 into hotwired-laravel:1.x Apr 7, 2023
@tonysm
Copy link
Collaborator

tonysm commented Apr 7, 2023

@tobyzerner you gave me a reason to test out the PHPBench tool and here's the result:

./vendor/bin/phpbench run --report=default
PHPBench (1.2.10) running benchmarks... #standwithukraine
with configuration file: /Users/tonymessias/Code/turbo-laravel/phpbench.json
with PHP version 8.2.1, xdebug ❌, opcache ❌

\ModelNamesBench

    benchWithoutCachingClassNames...........I0 - Mo3.596μs (±0.00%)
    benchWithCachingClassNames..............I0 - Mo0.252μs (±0.00%)

Subjects: 2, Assertions: 0, Failures: 0, Errors: 0
+------+-----------------+-------------------------------+-----+-------+------------+----------+--------------+----------------+
| iter | benchmark       | subject                       | set | revs  | mem_peak   | time_avg | comp_z_value | comp_deviation |
+------+-----------------+-------------------------------+-----+-------+------------+----------+--------------+----------------+
| 0    | ModelNamesBench | benchWithoutCachingClassNames |     | 10000 | 9,126,832b | 3.596μs  | +0.00σ       | +0.00%         |
| 0    | ModelNamesBench | benchWithCachingClassNames    |     | 10000 | 9,126,848b | 0.252μs  | +0.00σ       | +0.00%         |
+------+-----------------+-------------------------------+-----+-------+------------+----------+--------------+----------------+

It went from 3.596μs to just 0.252μs on 10,000 consecutive runs (revs).

@tonysm
Copy link
Collaborator

tonysm commented Apr 7, 2023

Tagged as 1.12.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonysm tonysm tonysm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tobyzerner @tonysm