FEATURE: Fusion performance optimization (lazy Component props) #2738

hlubek · 2019-10-16T15:50:41Z

What I did

Components provide a nice way to structure Fusion code but prevent lazy evaluation as Fusion does by default by eagerly evaluating all properties as props
If conditions are used inside the renderer, it's quite probable that not all props are used - so a large amount of unnecessary evaluations could be performed (we measured that to be in the range of 20-30% in a larger project)

How I did it

Introduced a LazyProps object implementing ArrayAccess that evaluates the actually accessed props lazily (and caches the results)

How to verify it

Components work as before (there could be edge-cases where explicit array type annotations are used for props)
props are only evaluated if used (verified by test)

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained branch

Resolves: #2793

hlubek · 2019-10-17T10:13:07Z

If changing props from array to ArrayAccess is too breaking (I noticed the demo site actually breaks with the SEO package), we could think about adding a @lazy attribute to Component to opt-in to this change (and make this the default in the next major version).

mficzel

While this change makes total sense and i would be fine with the ArrayAccess implementation. This still has issues in combination with @apply since this leads to nested LazyProxy sitautions. Those should be handled and tested aswell.

Example:

prototype(Vendor.Site:Example) < prototype(Neos.Fusion:Component) {
	
    applyValue = "example"
    applyError = Neos.Fusion:Fail

    renderer = afx`
    	<Foo:Bar applyValue={props.applyValue} applyError={props.applyError}/>
   	`  	
}

prototype(Foo:Bar) < prototype(Neos.Fusion:Component) {
    applyValue = null

    renderer = afx`
        <Foo:Baz {...props} />
    `
}

prototype(Foo:Baz) < prototype(Neos.Fusion:Component) {
    applyValue = null

    renderer = afx`
        :: {props.applyValue} ::
    `
}

Es expected the path applyError will never be evaluated which is good but also
applyValue will not end up in the Foo:Baz component which it did without the pr.

mficzel · 2019-10-21T18:22:32Z

Thinking about this further i come to the conclusion that @apply will have to learn how to deal with a lazy proxy.

hlubek · 2019-10-22T09:20:53Z

@mficzel Good point, I'll add a test to cover that case and try to fix it in the Runtime

mficzel · 2019-10-22T14:19:58Z

Would be great to make @apply support generic objects that implement ArrayAccess. That would allow to use DomainObjects for that in addition to the lazyProxy.

hlubek · 2019-10-22T15:25:48Z

@mficzel I think we'd need to change the way apply works by having some kind of chain (evaluate key -> isset on apply value 1 -> isset on apply value 2 -> isset in Fusion configuration), which might make things slower again... And we'd loose the possibility to enumerate/set all defined keys for array based implementations unless Traversable is implemented by the apply values.

mficzel · 2019-10-23T10:02:46Z

@hlubek i think by enabling the lazyEvaluationProx to know which keys exist without actually evluating them yet we can have both. This is the only thing @apply needs to decide wether it should try to read this.

hlubek · 2019-10-23T10:39:15Z

@mficzel This is what I did in aa168b7:

                    elseif ($singleApplyValues instanceof LazyProps) {
                        for ($singleApplyValues->rewind(); ($key = $singleApplyValues->key()) !== null; $singleApplyValues->next()) {
                            $combinedApplyValues[$fusionPath . '/' . $key] = [
                                'key' => $key,
                                'value' => function () use ($singleApplyValues, $key) {
                                    return $singleApplyValues[$key];
                                },
                                'lazy' => true
                            ];
                        }
                    }

Actually we could use Traversable and ArrayAccess as interfaces to check for this case, since these provide the necessary methods and array access to to the actual lazy evaluation. So every domain object implementing these interfaces would work (and having a dependency to FusionObjects/Helpers from Core is actually a code smell).

mficzel · 2019-10-23T11:15:15Z

I really like the idea to use ArrayAccess and Traversable and let LazyProps implement those.

mficzel · 2019-10-23T11:38:15Z

Neos.Fusion/Classes/FusionObjects/Helpers/LazyProps.php

+/**
+ * @Flow\Proxy(false)
+ */
+final class LazyProps implements \ArrayAccess, \Iterator


Does it make sense to add this as LazyEvaluationProxy to the Fusion/Core? That would allow to use it in other places aswell.

As this is no @api yet it also makes sense to think about moving it later.

I thought about it too, but this is rather specific for holding an array of keys and defering the evaluation of these. I'm not sure where and how we would re-use this. By only checking the interfaces in Runtime we always have the chance to extract a common class or something else later.

:-) i now have a usecase for that. Evaluating only the current page of a multipage form. But since i the fusion-forms shall support Neos 4.3 i will copy that code anyways but thanks for the inspiration.

hlubek · 2019-10-23T11:38:33Z

@mficzel It's now implemented like this. The failing test comes from UriBuilder type hints that are not reflected in the Fusion objects (null vs. empty array). Is somebody fixing this?

mficzel · 2019-10-23T11:41:54Z

@hlubek regarding the uri-builder there is a pr to fix it already #2733

mficzel · 2019-11-04T10:31:10Z

@hlubek the master is green again can you update the pr (rebase or merge master) #2737 is merged aswell which will probably also lead to a little rebasing

kitsunet · 2020-02-20T09:09:30Z

@hlubek sadly this has conflicts with the way Neos.SEO uses apply in the line
@apply.attributes = ${props.attributes ? Array.filter(props.attributes, attribute => attribute != null) : []}
If #1871 is merged this case should be no problem.

Would be great to test this change in a real-world project.

This links to the wrong change (because differetn repository), this neos/flow-development-collection#1871 makes more sense

Sebobo · 2020-04-03T06:03:44Z

@albe should be put this also into 5.2?

albe · 2020-04-03T10:55:05Z

Did not read through all the conversation, so I'm not sure if this is ready. But if you say it is, I'm happy to pull it into 5.2 now :)

hlubek · 2020-04-03T12:09:57Z

@albe I'd say it should be ready

albe · 2020-04-03T12:17:01Z

@mficzel can you take another look and/or remove your veto?

albe

Looks good codewise, not sure about the breakyness of added typehints in Fusion ComponentImplementation

albe · 2020-04-03T12:22:14Z

Neos.Fusion/Classes/FusionObjects/ComponentImplementation.php

@@ -51,37 +52,32 @@ public function evaluate()
     * @param array $context
     * @return array
     */
-    protected function prepare($context)
+    protected function prepare(array $context): array


Q: Is this supposed to be extended/inherited and hence the added typehints to be considered breaking?

This is not @api and no intended extension point. We only seperated this into a special method so we can address this in aspects seperately.

So for me this is ok.

mficzel

Finally tested again and could not construct a fusion problem with this approach any more. All nesting and rendering works as expected and i could not come up with an edge case to break this.

However there are two issues when debugging these new proxies that would be a problem for developers:

{Json.stringify(props)} >> returns empty (could be fixed by implementing JsonSearializable)
<Neos.Fusion:Debug>{props}</Neos.Fusion:Debug> >> runs into errors as the debug cannot be generated

I could force both by using {Array.concat(props,[])} instead of props but this is not a solution we can really recommend.

I have no idea how the second issue can be solved but it should be somehow.
@hlubek if you have an idea how to solve the debug issues i would really like to get this in.

bwaidelich · 2020-04-06T07:00:05Z

Neos.Fusion/Classes/FusionObjects/Helpers/LazyProps.php

+
+    public function offsetSet($path, $value)
+    {
+        // NOOP


Shouldn't that throw?

You mean the "set call" should throw an exception? Yes, we could do that.

I still think trying to set a lazy prop should throw :)

Yeah, let's do that IMO. Otherwise looks good :)

@albe It's a good idea to resolve all conversations before hitting the merge button. Otherwise open discussions might get lost – like this one.. :)

hlubek · 2020-04-08T15:16:50Z

Finally tested again and could not construct a fusion problem with this approach any more. All nesting and rendering works as expected and i could not come up with an edge case to break this.

👍 Love to hear that ;)

However there are two issues when debugging these new proxies that would be a problem for developers:
* `{Json.stringify(props)}` >> returns empty (could be fixed by implementing JsonSearializable)

Good idea, that should be easy to implement.

* `<Neos.Fusion:Debug>{props}</Neos.Fusion:Debug>` >> runs into errors as the debug cannot be generated
I could force both by using {Array.concat(props,[])} instead of props but this is not a solution we can really recommend.

I have no idea how the second issue can be solved but it should be somehow.
@hlubek if you have an idea how to solve the debug issues i would really like to get this in.

Maybe we can also use a test for instanceof JsonSerializable in the DebugImplementation or the Flow debug function itself and use that to extract "public" attributes.

mficzel · 2020-04-08T15:27:23Z

Maybe we can also use a test for instanceof JsonSerializable in the DebugImplementation or the Flow debug function itself and use that to extract "public" attributes.

Sounds smart, but i am not sure this will be accepted that easy as a json representation in the debug can hide things that were previously visible. Wouldnt it make more sense to make Debug use Iterator and ArrayAccess. We already have an exception for ArrayObject and DoctrineCollection here https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Error/Debugger.php#L200-L203

mficzel · 2020-04-08T16:19:07Z

@hlubek i think a have found a more generic approach for the debugging problem neos/flow-development-collection#1980 that would only leave the jsonSerialize open for this pr.

hlubek · 2020-04-20T12:02:51Z

@mficzel Thanks for the investigation regarding debugging. I added JsonSerializable to LazyProps and added a unit test for it. I also confirmed that Debugger now works as expected.

Should be good to go now.

mficzel

I like, thanks for this pr

mficzel · 2020-04-20T16:03:49Z

@albe @bwaidelich i think this is ready

albe · 2020-04-22T11:45:23Z

Thanks everyone! Very nice feature :)

hlubek · 2020-04-22T11:51:04Z

Thanks for reviewing and taking care!

starting with PHP 7.4 it's not cool to do offset access on integers (which returns null always) I did a little digging, and we have this skip keys logic 3 times in the runtime (probably not extracted for performance reasons) 2 times this logic needs to be adjusted to only operate on strings. Those two snippets where introduced with neos#2738 And 1 time this logic already has an is_string check. https://github.com/neos/neos-development-collection/blob/046e5d02750af0ebf33cc52663799ce09683ba37/Neos.Fusion/Classes/Core/Runtime.php#L667 This was adjusted when 7.4 compatibility was introduced via neos#2804 but the other two references where not adjusted

hlubek added 3 commits October 16, 2019 17:30

TASK: Fusion performance optimization (lazy Component props)

b7f9da9

Inlined fusionValue method

ad868a7

Implement test to verify lazy rendering

0f86e14

kitsunet self-requested a review October 16, 2019 16:04

Implement Traversable for conversion to array

4e61359

mficzel self-requested a review October 17, 2019 13:29

mficzel requested changes Oct 17, 2019

View reviewed changes

kdambekalns added P: Fusion T: PHP labels Oct 18, 2019

kdambekalns requested review from mficzel, jonnitto, kdambekalns and daniellienert and removed request for daniellienert October 18, 2019 15:13

Implement fix and test for apply with lazy props

aa168b7

Make StyleCI happy

db7c111

hlubek added 2 commits October 23, 2019 13:34

Use Traversable and ArrayAccess instead of LazyProps

c652338

Merge branch 'master' into task/fusion-performance-opt-lazyprops

9fcaa94

mficzel reviewed Oct 23, 2019

View reviewed changes

albe added this to In progress in Neos 5.2 & Flow 6.2 Release Board via automation Apr 3, 2020

albe reviewed Apr 3, 2020

View reviewed changes

albe changed the base branch from master to 5.2 April 4, 2020 12:09

mficzel requested changes Apr 5, 2020

View reviewed changes

Neos 5.2 & Flow 6.2 Release Board automation moved this from In progress to Reviews needed Apr 5, 2020

bwaidelich reviewed Apr 6, 2020

View reviewed changes

Make LazyProps JSON serializable

78f0b14

mficzel added 2 commits April 20, 2020 17:59

TASK: Add space that is required by styleci

1c61b5d

TASK: Add another space for styleCi

6982fb6

Neos 5.2 & Flow 6.2 Release Board automation moved this from Reviews needed to Reviewer approved Apr 20, 2020

mficzel approved these changes Apr 20, 2020

View reviewed changes

albe merged commit 24d8288 into neos:5.2 Apr 22, 2020

Neos 5.2 & Flow 6.2 Release Board automation moved this from Reviewer approved to Done Apr 22, 2020

albe changed the title ~~TASK: Fusion performance optimization (lazy Component props)~~ FEATURE: Fusion performance optimization (lazy Component props) Apr 22, 2020

albe mentioned this pull request Apr 29, 2020

TASK: Throw exception when lazy props are attempted to be changed #2971

Merged

mficzel mentioned this pull request Jun 30, 2020

Broken Rendering when applying Neos.Neos:ContentCollection as prop to to Neos.Fusion:Component renderer #3003

Closed

mhsdesign mentioned this pull request Oct 26, 2022

BUGFIX: Fusion Runtime @apply with integer indexed array #3936

Merged

6 tasks

FEATURE: Fusion performance optimization (lazy Component props) #2738

FEATURE: Fusion performance optimization (lazy Component props) #2738

Conversation

hlubek commented Oct 16, 2019 • edited by bwaidelich

hlubek commented Oct 17, 2019 • edited

mficzel left a comment

Choose a reason for hiding this comment

mficzel commented Oct 21, 2019

hlubek commented Oct 22, 2019

mficzel commented Oct 22, 2019

hlubek commented Oct 22, 2019 • edited

mficzel commented Oct 23, 2019 • edited

hlubek commented Oct 23, 2019

mficzel commented Oct 23, 2019

mficzel Oct 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlubek commented Oct 23, 2019

mficzel commented Oct 23, 2019

mficzel commented Nov 4, 2019 • edited

kitsunet commented Feb 20, 2020

Sebobo commented Apr 3, 2020

albe commented Apr 3, 2020

hlubek commented Apr 3, 2020

albe commented Apr 3, 2020

albe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mficzel left a comment • edited

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

hlubek commented Apr 8, 2020

mficzel commented Apr 8, 2020 • edited

mficzel commented Apr 8, 2020 • edited

hlubek commented Apr 20, 2020

mficzel left a comment

Choose a reason for hiding this comment

mficzel commented Apr 20, 2020

albe commented Apr 22, 2020

hlubek commented Apr 22, 2020

hlubek commented Oct 16, 2019 •

edited by bwaidelich

hlubek commented Oct 17, 2019 •

edited

hlubek commented Oct 22, 2019 •

edited

mficzel commented Oct 23, 2019 •

edited

mficzel Oct 23, 2019 •

edited

mficzel commented Nov 4, 2019 •

edited

mficzel left a comment •

edited

mficzel commented Apr 8, 2020 •

edited

mficzel commented Apr 8, 2020 •

edited