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

Performance impact of patterns in inserter when inserting into containers #28872

Open
david-szabo97 opened this issue Feb 9, 2021 · 2 comments
Labels
[Type] Performance Related to performance efforts

Comments

@david-szabo97
Copy link
Member

We introduced patterns inserter everywhere in this PR: #28459

Context

Before inserting a pattern, it checks whether the destination block supports all the root-level blocks from the patterns or not. This means we need to parse all the patterns before showing the inserter. This would cause the inserter to open slower. To avoid this, the PR above parses and memorizes all the parsed patterns on editor load. This increases the load time of the editor, but it doesn't affect the inserter's open time.

The problem

As I mentioned above, this PR increased the load time of the editor. Just for reference, here is the perf comparison from the CI checks:

post-editor

┌──────────────────┬──────────────────────────────────────────┬─────────────┐
│     (index)      │ dfdd19f5b9c7662acb045066ce734da1e3c65416 │   master    │
├──────────────────┼──────────────────────────────────────────┼─────────────┤
│       load       │               '9253.2 ms'                │ '9008.4 ms' │
│       type       │                '31.57 ms'                │ '30.86 ms'  │
│     minType      │                '22.77 ms'                │ '23.41 ms'  │
│     maxType      │                '80.46 ms'                │ '81.15 ms'  │
│      focus       │                '79.52 ms'                │ '83.72 ms'  │
│     minFocus     │                '62.09 ms'                │ '66.29 ms'  │
│     maxFocus     │               '116.37 ms'                │ '123.52 ms' │
│   inserterOpen   │               '619.49 ms'                │ '615.21 ms' │
│ minInserterOpen  │               '543.96 ms'                │ '549.59 ms' │
│ maxInserterOpen  │               '1007.11 ms'               │ '922.96 ms' │
│  inserterHover   │                '27.81 ms'                │ '27.41 ms'  │
│ minInserterHover │                '22.48 ms'                │ '22.43 ms'  │
│ maxInserterHover │                '39.92 ms'                │ '41.05 ms'  │
└──────────────────┴──────────────────────────────────────────┴─────────────┘

site-editor
┌─────────┬──────────────────────────────────────────┬───────────┐
│ (index) │ dfdd19f5b9c7662acb045066ce734da1e3c65416 │  master   │
├─────────┼──────────────────────────────────────────┼───────────┤
│  load   │               '2142.67 ms'               │ '2004 ms' │
└─────────┴──────────────────────────────────────────┴───────────┘

As you can see 9253 vs 9008 (~2.72%) and 2142 vs 2004 (~6%) it doesn't seem much. Roughly it takes 100ms in the CI checks to parse the patterns. This isn't much when loading the editor. But it would be a lot if it took 100ms longer to load the inserter for the first time. Don't forget, these tests were run using tt1-blocks which only contains 8 patterns. Let's assume it takes 15ms to parse 1 pattern. 10 patterns will take 150ms. 100 patterns will take 1.5 seconds. This will take even more time on a low-mid end computer and mobile.

Ideas

@vindl came up with a few ideas #28459 (comment)

  1. limit this functionality only to Site Editor for now. If we encounter performance problems in some cases the impact will be lower.
  2. save parsing results into IndexedDB so we don't need to do it on each load. Invalidation might be tricky here.
  3. something similar to (2) but doing this on backend by hooking into pattern registration, parsing it, persisting the value in a transient, and passing it to editor on init.
  4. lazy loading equivalent - just checking the patterns that are currently in view when inserter is open. This would likely be a bad UX since we might end up with inserter open and all of the patterns disabled, so I don't think it's a great approach.
@mtias mtias added the [Type] Performance Related to performance efforts label Feb 11, 2021
@mcsf
Copy link
Contributor

mcsf commented Feb 11, 2021

  1. save parsing results into IndexedDB so we don't need to do it on each load. Invalidation might be tricky here.
  2. something similar to (2) but doing this on backend by hooking into pattern registration, parsing it, persisting the value in a transient, and passing it to editor on init.

This feels overkill for a ~100 ms-long computation. At the same time, systematically adding those 100 ms to the initial load, regardless of a user's editing goals, isn't great.

A less-than-ideal but very simple improvement would be to cache it upon request, when the inserter is opened for the first time. A subsequent improvement might be to tap into requestIdleCallback to build that cache beforehand.

@david-szabo97
Copy link
Member Author

This is more of a long term brainstorming. Right now as you said, it's overkill. But as I mentioned, it's 100ms on my powerful MacBook pro, and we are only talking about only 8 patterns. As soon as we think about low-mid end laptops and a couple of dozen patterns then the performance impact quickly adds up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Performance Related to performance efforts
Projects
None yet
Development

No branches or pull requests

3 participants