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.

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

[11.x] Supercharge Blade #51143

Merged
merged 7 commits into from
Apr 22, 2024

Conversation

assertchris
Copy link
Contributor

@assertchris assertchris commented Apr 19, 2024

Attempting to cement my legend status, by:

  • Caching identical view renders
  • Only dispatching events if there are listeners
  • Inlining component props preparation steps
  • Avoiding needless array → AttributeBag → Iterator → array conversations
  • Caching known component constructor parameters for continuous exclusion

These changes focus on reducing engine work (not just in Blade) and not just by removing file I/O.

Seeing a 15% - 25% improvement (0% repeated attribute sets vs. 50% repeated attribute sets), with the following conditions:

  • opcache enabled throughout
  • 2000 components total
  • 2/3 user attributes per component
  • 1 benchmark iteration (obviously significant improvement with more iterations because of the in-memory cache, but that's not really useful)

Updates:

  • I removed the in-memory cache because I couldn't find a reliable and fast way to fingerprint view data

Fun anecdote:
I went as far as checking things like whether I could replace the preg_replace in Str::snake with custom str_replace voodoo, but benchmarking showed it wouldn't save much and it was...very strange.

@assertchris assertchris marked this pull request as draft April 19, 2024 19:23
Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@assertchris assertchris force-pushed the feature/supercharge-blade branch 2 times, most recently from c6e80d1 to f184d15 Compare April 19, 2024 22:01
@assertchris assertchris marked this pull request as ready for review April 19, 2024 22:04
@assertchris assertchris changed the title [WIP] Supercharge Blade Supercharge Blade Apr 19, 2024
@assertchris assertchris force-pushed the feature/supercharge-blade branch 3 times, most recently from 5f4d7cd to 4a06f2a Compare April 19, 2024 22:29
@assertchris assertchris changed the title Supercharge Blade [11.x] Supercharge Blade Apr 19, 2024
@assertchris assertchris marked this pull request as draft April 20, 2024 05:09
@assertchris assertchris marked this pull request as ready for review April 20, 2024 06:17
@driesvints
Copy link
Member

Tests fail here

@driesvints driesvints closed this Apr 22, 2024
@assertchris
Copy link
Contributor Author

assertchris commented Apr 22, 2024

I wanted to wait until someone with clout let me know they approved of the approaches before changing the test fixtures to match the new patterns in compiled views. Kinda disappointing that this is the first bit of feedback.

@driesvints
Copy link
Member

I'll re-open this but we normally don't review PR's with failing tests. We've had lots of issues in the past with PR's being merged with failing tests, sub sequentially causing other PR's to fail who were send after it being merged. As a precaution we simple do not accept PR's with failing tests.

@driesvints driesvints reopened this Apr 22, 2024
@assertchris assertchris marked this pull request as draft April 22, 2024 12:11
@assertchris
Copy link
Contributor Author

Not asking it to be merged with failing tests. Just some sort of 👍 for significant changes before I inflate the code to review with test changes. I will make the tests pass before taking it out of draft.

@driesvints
Copy link
Member

@assertchris Taylor doesn't sees draft PR's.

@assertchris assertchris marked this pull request as ready for review April 22, 2024 14:21
@lonnylot
Copy link
Contributor

@assertchris Your changes match a lot of the discovery I was doing.

In my tests the dispatch equated between 10-20%. So I'm curious what % of improvement in this PR can be attributed to the other changes.

@assertchris
Copy link
Contributor Author

@lonnylot Unsure. I think there could be value in standardising a benchmark arounds all sorts of component use-cases; which could be used to benchmark improvements. My benchmark was originally slanted towards better testing the in-memory cache use-case; but I ended up removing that part of the PR.

@taylorotwell taylorotwell merged commit ca54cb6 into laravel:11.x Apr 22, 2024
28 checks passed
@taylorotwell
Copy link
Member

Solid start! 🔥

@taylorotwell
Copy link
Member

Thanks @assertchris

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

Successfully merging this pull request may close these issues.

4 participants