Skip to content

Conversation

@apsinghdev
Copy link
Contributor

@apsinghdev apsinghdev commented Feb 1, 2025

fixes: #4305

This PR fixes the slow loading of Music Blocks.

(Draft)

This PR will contain comparatively bigger changes. So I'll keep on adding commits here to describe what they do.

Current Stats

Screenshot 2025-02-01 at 2 02 51 PM

Total loading time - ~ 16.0 seconds

cc @walterbender @pikurasa

Changes

82bf4ca formats the index.html file as it was not formatted properly.
4ffa134 add header links in optimized order to efficient loading
10723b3 fixes the render-blocking issue and bring the cumulative-layout-shift score from 0.20 to 0.07
5084cf7 implements http-server replacing the SimpleHTTPServer. Upgrades HTTP from 1.0 to 1.1 with Keep-Alive enabled
e6ad80f reduces Largest Contentful Paint element from 4,297 ms to 2,400 ms

@omsuneri
Copy link
Member

omsuneri commented Feb 1, 2025

@apsinghdev I would love to contribute to this PR can you suggest me some reference

@apsinghdev
Copy link
Contributor Author

@apsinghdev I would love to contribute to this PR can you suggest me some reference

Thanks @omsuneri. I'll let you know if I need help. 🙂

@walterbender
Copy link
Member

This is already a big improvement.

One minor issue: the splash screen is no longer centered horizontally.

Screenshot From 2025-02-01 11-29-38

@apsinghdev
Copy link
Contributor Author

@walterbender Thanks for letting me know. Don't know why it didn't appear on my browser. Anyways, next task is to optimise this video as it's blocking the other ops. It's causing Largest-content-page to load in around 4530 ms.

@apsinghdev apsinghdev force-pushed the perf/musicblocks-loading branch from 10723b3 to 14518ba Compare February 12, 2025 11:53
@apsinghdev
Copy link
Contributor Author

Update:

fixed the issue: #4331 (comment)

Experimented running MB using http-server

We are using SimpleHTTPServer to run Music Blocks to the client. But, SimpleHTTPServer doesn't support advanced features like Keep-Alive. I have experimented with running MB using http-server and it has Keep-Alive by default.

Screenshot 2025-02-15 at 9 22 14 PM

Also, I found http-server is comparatively fast increasing performance scores from 70 to 75 on the lighthouse.

Screenshot 2025-02-15 at 9 30 06 PM

Getting some feedback on these ideas. Will push it soon!

cc @walterbender @pikurasa

@walterbender
Copy link
Member

maybe https-server?

@apsinghdev apsinghdev force-pushed the perf/musicblocks-loading branch from 14518ba to 5084cf7 Compare February 16, 2025 07:56
@apsinghdev
Copy link
Contributor Author

maybe https-server?

I've replaced SimpleHTTPServer with http-server. Also as we don't need python now to run MB, I have made some changes in docs as well. Please test this commit 5084cf7 to your side.

@apsinghdev
Copy link
Contributor Author

apsinghdev commented Feb 16, 2025

@walterbender I've used a custom server using express that gives us the freedom to customize things as we need and make Music Blocks really fast with its default features.

I tested it and it is showing really good performance.

Screenshot 2025-02-16 at 2 28 27 PM

If you get a chance, please test 62397de

@walterbender
Copy link
Member

Seems good to me.

@apsinghdev apsinghdev force-pushed the perf/musicblocks-loading branch from 62397de to e6ad80f Compare February 17, 2025 11:01
@apsinghdev
Copy link
Contributor Author

Seems good to me.

Thanks! I'll do more testing.

@apsinghdev apsinghdev force-pushed the perf/musicblocks-loading branch from e6ad80f to 901de5c Compare February 18, 2025 08:26
@apsinghdev apsinghdev marked this pull request as ready for review February 18, 2025 08:26
@apsinghdev
Copy link
Contributor Author

apsinghdev commented Feb 18, 2025

@walterbender, I've fixed most of the critical issues blocking MB's loading process. I've noticed two main issues that need to be worked on:

  1. Unoptimized DOM-related code: we've lots of unoptimized code that interacts with DOM leading to delays in loading.
  2. Outdated/Unoptimized libraries: Some of the libraries we are using contain unused/deprecated code and it slows the performance.

To tackle both of these issues we'd need to plan about it as these are big breaking changes that will impact the whole codebase.

For now, I've made this PR ready and will work on this issue as we discuss further. Please have a look.

@pikurasa
Copy link
Collaborator

@apsinghdev The video perfectly fills the screen now. However, the loading text is now missing.

As for load time, I tested the master branch and 62397de with the default Sol, Mi, Sol code. Both took 13 seconds to load.

@apsinghdev
Copy link
Contributor Author

apsinghdev commented Feb 19, 2025

@pikurasa Thanks for your feedback! I'll fix the missing loading text issue. Also, which tool have you used to calculate the load time?

@pikurasa
Copy link
Collaborator

@pikurasa Thanks for your feedback! I'll fix the missing loading text issue. Also, which tool have you used to calculate the load time?

A simple phone timer.

@apsinghdev
Copy link
Contributor Author

@pikurasa I have tested the changes and it's varying from 9 to 12 seconds. Caching is also a factor that affects the speed on different machines. Also the major reason that is slowing down the loading is the one that I've mentioned in this comment #4331 (comment) -Unoptimized DOM. Probably, I'll work on it in another PR.

@apsinghdev apsinghdev force-pushed the perf/musicblocks-loading branch from 901de5c to 83f797c Compare February 25, 2025 17:24
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

1 similar comment
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@apsinghdev
Copy link
Contributor Author

apsinghdev commented Feb 25, 2025

@pikurasa I have fixed the loading text issue.
Screenshot 2025-02-25 at 10 52 14 PM

@apsinghdev
Copy link
Contributor Author

@walterbender I guess, this PR is ready to be merged. Will create a new ticket for DOM improvements and will raise PRs in that. Please let me know if I need to change something.

@apsinghdev
Copy link
Contributor Author

@walterbender Is there any feedback on this PR?

@apsinghdev apsinghdev force-pushed the perf/musicblocks-loading branch from 03c48da to 688bb92 Compare March 2, 2025 16:47
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2025

✅ All Jest tests passed! This PR is ready to merge.

@walterbender
Copy link
Member

This seems to really help with loading times. I need to do some more testing, but all-in-all, big improvement. Thx.

@pikurasa
Copy link
Collaborator

pikurasa commented Mar 2, 2025

I tested it, and it works.

As for performance, it seems to improve load time by a second, at least for running it locally. (I wish I had a more robust way to test this for you...)

@walterbender walterbender merged commit f3df4ca into sugarlabs:master Mar 2, 2025
5 checks passed
@apsinghdev
Copy link
Contributor Author

@walterbender @pikurasa Thank you! I'll perform more tests while working on the DOM issue as both of these issues are closely related. Also, this PR has addressed the issues that were needed to be resolved before working on the DOM issue.

@apsinghdev apsinghdev deleted the perf/musicblocks-loading branch March 3, 2025 04:17
@justin212407 justin212407 mentioned this pull request Mar 4, 2025
4 tasks
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.

Performance: Improve how MB handles local cache

4 participants