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

Cache block item constructors for block based editors #15264

Merged

Conversation

kjac
Copy link
Contributor

@kjac kjac commented Nov 20, 2023

Prerequisites

  • I have added steps to test this contribution in the description below

Description

When block based editors construct their published values, they use a whole lot of reflection and on-the-fly IL generated types to construct their contained block items. In short, they activate a lot of what's going on in ReflectionUtilities.

Unfortunately the reflected results are not cached between requests, even though they can never differ as they are type bound. In fact, the results are not even cached between block based editors, so if two block based editors exist on the same page one or more of the same block types, the reflection is repeated for each editor. This also even applies to nested block editors.

The block editors do cache their published values, but all this reflection will still happen for each "first page render" (and after a page publish). The result is a fair amount of CPU time spent churning reflection:

image

This PR aims to cache as much of this reflection as possible, so we don't have to do all of this reflection over and over.

Since block items are typed to the individual block based editors, caches must be introduced and isolated for each of them (BlockListItem<TContent, TSettings>, BlockGridItem<TContent, TSettings> and RichTextBlockItem<TContent, TSettings> respectively).

Testing this PR

To test this PR, we must ensure that the Block List, the Block Grid and the Rich Text Editor all continue to work the same for their contained blocks. This means they must work:

  • With ModelsBuilder disabled having no generated models built into the site.
  • With ModelsBuilder disabled having some generated models built into the site.
  • With ModelsBuilder disabled having all generated models built into the site.
  • With ModelsBuilder enabled in all modes.
  • With ModelsBuilder running "SourceCode" modes having no generated models built into the site.
  • With ModelsBuilder running "SourceCode" modes having some generated models built into the site.
  • With ModelsBuilder running "SourceCode" modes having all generated models built into the site.

...and probably a few other permutations.

It might be best to reach out to @kjac for testing 😆

Copy link
Contributor

@Migaroez Migaroez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Looks good
✅ Pair testing seems to cover everything.

@Migaroez Migaroez merged commit 88f9079 into release/13.0 Nov 29, 2023
12 of 13 checks passed
@Migaroez Migaroez deleted the v13/fix/constructor-cache-for-block-based-editors branch November 29, 2023 11:27
@Nuklon
Copy link
Contributor

Nuklon commented Dec 11, 2023

This isn't thread safe, see #15421

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.

3 participants