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

TemplateInserter extremely slow / lagging #2577

Closed
swissspidy opened this issue Jun 12, 2019 · 10 comments · Fixed by #2689
Closed

TemplateInserter extremely slow / lagging #2577

swissspidy opened this issue Jun 12, 2019 · 10 comments · Fixed by #2689
Assignees
Labels
Bug Something isn't working
Projects
Milestone

Comments

@swissspidy
Copy link
Collaborator

When clicking on the button to open the template inserter in the stories editor, it takes more than a second until something happens.

When the template inserter finally opens, you cannot immediately scroll through the template list as some assets (background images, etc.) are still being loaded.

At this point, you'll see incomplete template previews because some images are still missing and fonts are being enqueued.

After that, finally, the template inserter is fully functional!

--

A possible solution would be to how a spinner while the templates are being fetched in order to not delay any UI interactions.

For something like #2012 we'd need such a spinner anyway.

@swissspidy swissspidy added Bug Something isn't working AMP Stories labels Jun 12, 2019
@swissspidy swissspidy added this to Backlog in AMP Stories Jun 12, 2019
@westonruter
Copy link
Member

I noticed this as well.

One thing to improve this would be to render a template only as it comes into view. Lazy loading.

Another thing that comes to mind is ensuring that images in the editor have decoding="async".

@swissspidy
Copy link
Collaborator Author

AFAIK react-virtualized and react-window are the current de facto standards for this kind of lazy loading in React. Might be worth trying these out. Although I think at the moment for ~ 10 templates it's not an urgent thing yet.

As for decoding="async" that would be an interesting thing to do, but it could be tricky to apply to all the images in these nested React components.

@MackenzieHartung MackenzieHartung moved this from Backlog to To Do in AMP Stories Jun 12, 2019
@MackenzieHartung MackenzieHartung added this to the v1.2.1 milestone Jun 12, 2019
@miina miina self-assigned this Jun 24, 2019
@miina miina moved this from To Do to In progress in AMP Stories Jun 24, 2019
@miina
Copy link
Contributor

miina commented Jun 25, 2019

Looks like lazy loading seems to be the best option here, will look into the options that you suggested, @swissspidy. react-window together with react-window-infinite-loader could be an option.

The delay seems to come only from the BlockPreview and not from fetching the templates -- the fetching happens already when the TemplateInserter is mounted (not when clicking) and at least locally testing it seems that the templates are already loaded when clicking the icon happens.

Most of the images come from background-image CSS so async decoding doesn't seem to be a valid option either, also, as Pascal mentioned, it would be tricky to actually implement it.

Additionally, the background images were already converted to JPEG within 3720900 and scaled smaller within 7e84388 but we can probably reduce the image sizes a bit more as well (since we're only using the thumbnails anyway, at least right now).

@swissspidy
Copy link
Collaborator Author

I see, thanks for clarifying! So it's just a render that's blocking the main thread I guess.

I've also asked about this on Slack. While there was no immediate answer, it was suggested that this would be a good use case for the upcoming Concurrent Mode in React.

@miina
Copy link
Contributor

miina commented Jun 27, 2019

Yep, saw your question on Slack, thanks for asking about that. Also checked into concurrent mode before -- would be a good use case for that, maybe it would be good to add a @todo comment to the code for future reference as well.

@miina miina moved this from In progress to Ready for Review in AMP Stories Jun 27, 2019
@swissspidy swissspidy moved this from Ready for Review to Ready for QA in AMP Stories Jul 1, 2019
@kienstra
Copy link
Contributor

kienstra commented Jul 4, 2019

Request For Testing

Hi @csossi,
Could you please test this?

Steps:

  1. Create a new story
  2. Click the 'Template Inserter'
  3. Expected: a 'spinner' appears on loading the templates:

template-inserter

4. Expected: the templates load relatively quickly, though this isn't an easy issue to fix

@kienstra kienstra assigned csossi and unassigned miina Jul 4, 2019
@csossi csossi removed their assignment Jul 7, 2019
@csossi csossi moved this from Ready for QA to Ready for Merging in AMP Stories Jul 7, 2019
@csossi csossi moved this from Ready for Merging to Ready for QA in AMP Stories Jul 10, 2019
@csossi csossi self-assigned this Jul 10, 2019
@csossi
Copy link

csossi commented Jul 13, 2019

On first attempt (new page - add template), the loader animation DOES appear but when it leaves the display, the templates haven't quite loaded in full yet (user doesn't have cursor control). User has to wait upwards of 10 seconds in total before they can begin to navigate/select within template section.

When user tries to add additional pages, loading animation appears and section loads in a second or two

@csossi csossi assigned miina and unassigned csossi Jul 13, 2019
@csossi csossi moved this from Ready for QA to In progress in AMP Stories Jul 13, 2019
@miina
Copy link
Contributor

miina commented Jul 15, 2019

I'm not seeing this issue when testing, both locally and in the test environment on the first attempt after the Spinner stops showing the templates take less than a second to become available for use. Could it be a connection issue?

@swissspidy Could you confirm if you still see the templates loading within a reasonable time or is it loading slow again?

@swissspidy
Copy link
Collaborator Author

The only slowness I can see is when images are being loaded (after the spinner disappears). There's not much we can do about that I think.

@miina
Copy link
Contributor

miina commented Jul 15, 2019

Same here, I'll just move this to Ready for Merging for now.

@miina miina moved this from In progress to Ready for Merging in AMP Stories Jul 15, 2019
@swissspidy swissspidy moved this from Ready for Merging to Done in AMP Stories Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants