Top page feature (performance enhancement) #774

mfendeksilverstripe · 2020-02-06T20:55:35Z

Top page feature (performance enhancement)

Top page feature is a performance enhancement designed especially for projects with deeper nesting of objects. The deeper the nesting the higher performance gain.

This feature should be Virtual Blocks compatible.

Performance issues

Currently, the bottom up data lookups (from block to page) are slow or almost impossible in some cases (when using Fluent and locale us unknown). These are the most common cases of such lookups:

CMS fields - block level CMS fields depend on page data
templates - block level templates logic depends on page data
dev tasks - dev tasks needs to match block to pages (starting from block context)

Deeply nested data structures may not be that uncommon either. Consider this example:

Page
  ElementalArea
    RowBlock (represents grid row on frontend)
     ElementalArea
       AccordionBlock (block which can contain other content blocks)
         ElementalArea
           ContentBlock

This module supports following bottom up data lookups:

ElementalArea::getOwnerPage() - this method returns most immediate owner object not necessarily the top page, the method is cached in-memory but needs to be called several times on different objects when traversing bottom up. In our example above this method would return AccordionBlock when calling from deepest located elemental area
BaseElement::getPage() - this method is just a cached (in-memory) proxy to the ElementalArea::getOwnerPage() so it doesn't necessarily return the top page

Solution

This feature stores the page reference on every block and every elemental area and it provides a simple API to lookup the top page. This feature is duplication safe. Fluent module integration support is available as well.

Other changes

minimum PHP version bumped up to 7.1
minor linting changes

Related issues

#775

Cheddam · 2020-02-09T23:37:27Z

Hey @mfendeksilverstripe, thanks so much for raising this PR 😄 I'll try to get it reviewed soon, but in the meantime:

I've marked this as change/minor, since it seems to be opt-in new API surface, rather than a breaking change to existing behaviour. If that's correct, I'd recommend rebasing the PR onto the 4 branch, otherwise I'm keen to get a bit more context on what it breaks and why 👍

mfendeksilverstripe · 2020-02-10T21:30:21Z

@Cheddam Thanks for the feedback, rebased on 4

phptek · 2020-02-11T01:18:43Z

docs/en/advanced_setup.md

+```
+DNADesign\Elemental\Models\BaseElement:
+  extensions:
+    topPage: DNADesign\Elemental\TopPage\FluentExtension


Suggestion: Do we need to use the associative array syntax? This will mess with almost every extension YML config I've seen which use indexed arrays for their extension syntax. If developers try to mix the two, the Symfony YML parser will barf. Also note Silverstripe's order of precedence when it comes to applying extensions. AFAIK, if developers attempt to declare >1 extension on a given class in a separate YML block, then only the latter will take and the former are clobbered.

phptek · 2020-02-11T01:23:54Z

docs/en/advanced_setup.md

+Instead, the newly created data objects will have a new top page data assigned based on the context.
+
+For example, duplicating a page with all of its blocks will create a new page and new blocks.
+All the new blocks will have the new page stored as their top page reference. This works even if the deplication tree contains other pages as tree nodes.


Typo: "duplication tree" or possibly what was meant was "duplicated tree"?

Cheddam

Hey Mojmir, I've taken a look over the code and it looks good - the only suggestions I've made are some tidying of the language used in the docs and docblocks.

Regarding the block sharing limitation - have you tested this solution with Virtual Blocks? If that worked with this code, it'd provide an alternative to directly sharing blocks between pages for projects that want to adopt this functionality.

My biggest question is: Does this make more sense as a separate module? It's a lot of additional code to add a fairly specific feature, and it can't add this functionality by default, because it might impact existing usages of the module.

We could look at deprecating the paradigm of directly sharing blocks, and shift this PR back to targeting the next major and enabled by default, but I'm not sure what portion of real projects use block sharing and would be impacted. For projects that do use that paradigm, upgrading could require a lot of refactoring and migration work for them to shift to Virtual Blocks, and at worst it may simply be impossible to upgrade, leaving those sites at a dead-end. There are no immediate plans to release a new major, either, so this functionality would be sitting in master without a tag for a while.

If this was a separate module, we could add it to the suggested package config for Elemental to increase its visibility as a drop-in performance enhancement, and developers could evaluate and adopt it immediately.

These are just my thoughts on the matter, and I'm keen to get some input from other maintainers - cc/ @ScopeyNZ @kinglozzer @maxime-rainville.

I really appreciate the work you've put into this, and I'm excited for it see the light of day in whatever form it takes!

Cheddam · 2020-02-20T21:52:41Z

src/TopPage/DataExtension.php

+ * Top page data cache for improved performance
+ * intended owners of this extension are @see BaseElement and @see ElementalArea
+ * applying this extension to just one of these owners will not hinder top page functionality
+ * but the performance gain will be smaller
+ * it is recommended to apply this extension to BaseElement and for setups with deeper block nesting
+ * it is recommended to cover ElementalArea as well


Could be simplified to:

* Provides a db-cached reference to the top-level page for improved read performance on projects * with deeply nested block structures. Apply to @see BaseElement and @see ElementalArea.

(Not sure if there's a reason to explain why only applying it to one or the other impacts its usefulness - are there any downsides in applying it to both?)

The downside is that you have to store more data in the DB and it may not be necessary if you have a flat block page (i.e. only one elemental area). Maybe the downside is acceptable in all cases and I should simplify the explanation.

Cheddam · 2020-02-20T22:05:03Z

src/TopPage/DataExtension.php

+     * Find top level page of a block or elemental area
+     * this is very useful in case blocks are deeply nested
+     *
+     * for example:
+     * page -> elemental area -> block -> elemental area -> block
+     *
+     * this lookup is very performant as is safe to use in a template as well


Could simplify to:

* Finds the top-level Page object for a Block / ElementalArea, using the cached TopPageID * reference when possible.

Cheddam · 2020-02-20T22:09:06Z

src/TopPage/FluentExtension.php

+ * Use this extension in case you use the Fluent module (https://github.com/tractorcow-farm/silverstripe-fluent)
+ * for page localisation
+ * this will keep track of the locale the nested data object is stored in


Could be simplified to:

* Use in place of @see DataExtension if you use the Fluent module for page localisation. * * @link https://github.com/tractorcow-farm/silverstripe-fluent

Cheddam · 2020-02-20T22:12:42Z

src/TopPage/SiteTreeExtension.php

+ * This extension is mandatory for any pages that need to support top page functionality
+ * it is safe to apply this extension directly to Page as the functionality detects presence of elemental area
+ * alternatively, this extension can be applied to top level block page or all block pages individually
+ * in case no common parent exists


Could be simplified to:

* This extension must be present on pagetypes that need to support Elemental TopPage functionality. * It can be applied directly to Page, as it only takes effect in the presence of a ElementalArea.

Cheddam · 2020-02-20T22:59:05Z

src/TopPage/SiteTreeExtension.php

+     * This is a very roundabout way on how to update top page reference for data objects
+     * the main reason why we're doing it like this is the fact that duplication process goes top-down
+     * when it comes to model creation, but it goes bottom-up when it comes to writes
+     * as a consequence the relations are not available when model is written
+     *
+     * Instead of updating the page reference during model write, we will simply push the object into a list
+     * and update the reference later when page is available
+     *
+     * note that when going top-down on the relationship tree the models are not written yet so we can't use
+     * page IDs as identifiers
+     * instead we use IDs of the original objects that we are cloning from
+     *
+     * Additionally, the duplication process has to support nested pages which makes things more complicated
+     * this is handled by using a stack like data structure to keep track on multiple pages
+     * each with it's own duplication list


Could be simplified to something along these lines:

* Registers the given object to receive an updated TopPage reference after the duplication * operation completes, ensuring the new Page is written to the database beforehand. * * The registry uses a stack-like structure to allow accurate tracking of objects during * duplication operations that include nested pages.

(Feel free to adjust if I've misinterpreted anything, or missed crucial information.)

Cheddam · 2020-02-20T23:15:49Z

docs/en/advanced_setup.md

+
+The page reference data on the blocks can also be used for maintenance dev tasks as it's easy to identify which blocks belong to which pages in which locale.
+
+### Top page reference data during object duplication


This section might not be necessary - I'd see it being useful if it was describing a limitation, but instead it's just reassuring the developer that the code will work as expected.

Cheddam · 2020-02-21T02:03:20Z

src/TopPage/DataExtension.php

+        $extension->addDuplicatedObject($this->owner);
+    }
+
+    protected function assignTopPage(Page $page): void


Needs a docblock

Cheddam · 2020-02-21T02:03:36Z

src/TopPage/FluentExtension.php

+        'TopPageLocale' => 'Varchar',
+    ];
+
+    protected function assignTopPage(Page $page): void


Needs a docblock

Cheddam · 2020-02-21T02:03:55Z

src/TopPage/SiteTreeExtension.php

+        $this->processDuplication($original, (bool) $doWrite);
+    }
+
+    public function getDuplicationKey(): ?string


Needs a docblock

Cheddam · 2020-02-21T02:04:10Z

src/TopPage/SiteTreeExtension.php

+        array_unshift($this->duplicatedPages, $key);
+    }
+
+    protected function processDuplication(Page $original, bool $written): void


Needs a docblock

mfendeksilverstripe · 2020-02-24T00:57:48Z

@Cheddam Thank you for your feedback, I pushed up some documentation changes.

I had a look at the Virtual Blocks module and I think that this feature will work with it just fine.

I do recommend to look into deprecating the shared blocks approach and recommend the use of Virtual Blocks.

I'm not very keen on moving this to a separate module as this is just a performance fix. The performance is painfully slow in a deeply nested blocks setup and I see this as a severe issue of this module which should be fixed in this module.

I will look into updating the feature to make it enabled by default and not break when a project is using shared blocks.

mfendeksilverstripe · 2020-02-24T01:24:17Z

@Cheddam As far as I can see the shared blocks feature is no longer there as Elemental area has many blocks, but block only has one parent. I enabled this performance improvement by default and rebased on 4.

mfendeksilverstripe · 2020-02-24T23:13:13Z

@Cheddam All tests are now green :)

Cheddam · 2020-03-16T03:54:55Z

@mfendeksilverstripe After discussion with community members on Slack, I've made an adjustment to disable the extensions by default, and reintroduced the documentation describing how to enable them. If you're happy with the change, we can merge and then raise a follow-up PR to master in order to enable it there by default.

mfendeksilverstripe · 2020-03-16T17:58:03Z

@Cheddam Thanks I'll have a look at it today and I'll try to fix the unit tests ;-)

mfendeksilverstripe · 2020-03-16T23:15:01Z

@Cheddam Your changes look fine but I'm not sure why the Behat tests are now failing. Note that the tests passed before your change. Please look into it.

Cheddam · 2020-03-26T01:43:16Z

The behat failure was caused by an upstream issue, which has since been resolved, so CI is now green 👍

mfendeksilverstripe force-pushed the feature/page-aware branch from 768daa0 to d43a9f4 Compare February 6, 2020 20:56

mfendeksilverstripe mentioned this pull request Feb 6, 2020

Performance issues with nested blocks #775

Closed

mfendeksilverstripe force-pushed the feature/page-aware branch from d43a9f4 to a8bb500 Compare February 6, 2020 21:32

mfendeksilverstripe requested a review from Cheddam February 6, 2020 21:51

Cheddam added affects/v4 change/minor complexity/medium impact/medium type/enhancement labels Feb 9, 2020

mfendeksilverstripe force-pushed the feature/page-aware branch from a8bb500 to f873bc8 Compare February 10, 2020 21:29

mfendeksilverstripe changed the base branch from master to 4 February 10, 2020 21:29

phptek reviewed Feb 11, 2020

View reviewed changes

Cheddam requested changes Feb 21, 2020

View reviewed changes

mfendeksilverstripe force-pushed the feature/page-aware branch from 962faf7 to 5efdb6a Compare February 24, 2020 01:19

Top page feature

e87728d

mfendeksilverstripe force-pushed the feature/page-aware branch from c8ce31b to e87728d Compare February 24, 2020 22:56

Cheddam approved these changes Mar 16, 2020

View reviewed changes

NEW Disable TopPage extensions by default

20e1c3e

mfendeksilverstripe force-pushed the feature/page-aware branch from 8adec59 to 20e1c3e Compare March 16, 2020 23:09

Cheddam merged commit 4704380 into silverstripe:4 Mar 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Top page feature (performance enhancement) #774

Top page feature (performance enhancement) #774

mfendeksilverstripe commented Feb 6, 2020 •

edited

Loading

Cheddam commented Feb 9, 2020

mfendeksilverstripe commented Feb 10, 2020

phptek Feb 11, 2020 •

edited

Loading

phptek Feb 11, 2020

Cheddam left a comment

Cheddam Feb 20, 2020

mfendeksilverstripe Feb 23, 2020 •

edited

Loading

Cheddam Feb 20, 2020

Cheddam Feb 20, 2020

Cheddam Feb 20, 2020

Cheddam Feb 20, 2020

Cheddam Feb 20, 2020

Cheddam Feb 21, 2020

Cheddam Feb 21, 2020

Cheddam Feb 21, 2020

Cheddam Feb 21, 2020

mfendeksilverstripe commented Feb 24, 2020

mfendeksilverstripe commented Feb 24, 2020

mfendeksilverstripe commented Feb 24, 2020

Cheddam commented Mar 16, 2020

mfendeksilverstripe commented Mar 16, 2020

mfendeksilverstripe commented Mar 16, 2020

Cheddam commented Mar 26, 2020


		The page reference data on the blocks can also be used for maintenance dev tasks as it's easy to identify which blocks belong to which pages in which locale.

		### Top page reference data during object duplication

Top page feature (performance enhancement) #774

Top page feature (performance enhancement) #774

Conversation

mfendeksilverstripe commented Feb 6, 2020 • edited Loading

Top page feature (performance enhancement)

Performance issues

Solution

Other changes

Related issues

Cheddam commented Feb 9, 2020

mfendeksilverstripe commented Feb 10, 2020

phptek Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cheddam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfendeksilverstripe Feb 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfendeksilverstripe commented Feb 24, 2020

mfendeksilverstripe commented Feb 24, 2020

mfendeksilverstripe commented Feb 24, 2020

Cheddam commented Mar 16, 2020

mfendeksilverstripe commented Mar 16, 2020

mfendeksilverstripe commented Mar 16, 2020

Cheddam commented Mar 26, 2020

mfendeksilverstripe commented Feb 6, 2020 •

edited

Loading

phptek Feb 11, 2020 •

edited

Loading

mfendeksilverstripe Feb 23, 2020 •

edited

Loading