Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: improve first load time #179

Merged
merged 1 commit into from
Jan 28, 2022
Merged

fix: improve first load time #179

merged 1 commit into from
Jan 28, 2022

Conversation

Rigby90
Copy link
Contributor

@Rigby90 Rigby90 commented Jan 28, 2022

Appreciate your work on this project, using it in numerous Laravel projects myself.

However, I've found myself running a local fork of the project which contains the changes in this PR as I had noticed, at least on the machines I develop with (Windows) there appears to be an issue with the load times when building the Blade template / HTML. On average the load time is around 6~ seconds but in some cases has taken well over 10 seconds for the initial page load.

With regards to the changes -

I noticed an unused variable existed that looked like the intention was to cache the result of the 'isDevelopmentServerRunning' result, so I've gone and implemented this which reduces the 6-10 second load times I was experiencing down to 2~ seconds, as each load calls the 'isDevelopmentServerRunning' function at least 3 times.

However, I looked further into why the request was still taking 2-3~ seconds to initially load and had narrowed it down to the fsockopen call. Doing a bit of research seems to suggest that this call does not use any cached DNS responses from the OS but instead makes a DNS request to the nameserver each time it is called. Swapping to using 'gethostbyname' to resolve the hostname prior to the fsockopen function bought the initial function call time down from >2s to <100ms.

Both changes bring the page load time on the current project I'm working on down from 7s to 800-900ms on average.

src/Vite.php Outdated Show resolved Hide resolved
@innocenzi
Copy link
Owner

Looks good, looks like the mock server in tests doesn't work anymore though. You're right about the cache variable (how did I miss that), but I was thinking about using the Http client from Laravel in the next version. That would clean up tests as well. Have you tried it?

@Rigby90
Copy link
Contributor Author

Rigby90 commented Jan 28, 2022

With regards to the tests failing, I'm going to make the assumption that this is perhaps related to how the hostname lookup was changed, I'm not overly familiar with how GitHub Actions networking works so perhaps gethostbyname was returning a different ip address to what the fsockopen was getting as fsockopen queries nameservers but gethostbyname I believe uses the local hostnames, then falls back to a DNS query.

However, with that being said, your idea of moving to the Laravel HTTP Client sounded like a much better way of handling this and does indeed maintain the performance improvements. I've gone and updated my PR to incorporate these changes.

One thing that became apparent during testing of the Laravel HTTP Client, is that the Mock server always returns a status 200, even though an asset does not exist whereas Vite returns a 404. Although I don't believe it's causing any false results with the existing tests it threw me off when swapping to the HTTP Client as I was checking the status codes initially and was getting mixed results between the tests and a development environment.

Lastly, all tests are currently passing locally on Windows & WSL, so hopefully, the GA will pass this time around as well with the change to DNS lookup.

@innocenzi
Copy link
Owner

Let's do like this for now and I'll update tests properly in #176, thanks!

@innocenzi innocenzi changed the title Resolve Slow First Load fix: improve first load time Jan 28, 2022
@innocenzi innocenzi merged commit d1c69ed into innocenzi:main Jan 28, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants