FEATURE (performance): compress SetSerializedNodeProperties together; allow experiments in codebase #4263

skurfuerst · 2023-05-05T13:04:29Z

When people write texts, the Neos UI will send many SetSerializedNodeProperties commands touching the same node.

This change compacts this to a single SetSerializedNodeProperties command, so that projection rebase is sped up.

TODO: This is not yet well tested, and I do not like the code for the compaction algorithm so far.

When people write texts, the Neos UI will send many SetSerializedNodeProperties commands touching the same node. This change compacts this to a single SetSerializedNodeProperties command, so that projection rebase is sped up. TODO: This is not yet well tested, and I do not like the code for the compaction algorithm so far.

bwaidelich · 2023-05-24T08:31:07Z

I like the idea, but I have the feeling that we should go for a more generic approach here (I know the caveats of generic approaches, but IMO in this case it's worth it).

Also I wonder, if that "compraction" should be done on the events instead (as we won't have to replay commands at some point hopefully).
Maybe we can also combine this logic with #4284 (i.e. two disabled events can be compracted to one)

skurfuerst · 2023-05-24T11:15:09Z

Yeah this PR is merely a draft; although speeding up SetNodeProperties is particularly performance relevant because the UI sends LOTS of these commands during normal editing (and right now we preserve them until live).

So I am not fully sure on a totally generic solution, because this solution actually solves 95% of the Neos UI pain :)

All the best,
Sebastian

…mental stuff

bwaidelich

Looking great, just some minor remarks.

Apart from those I am not sure whether we need the experiment "cateogories" (at this point in time):
i.e. you added the "compress-simple" experiment as some kind of sub-category of "compactCommands" but I am not sure if it makes things easier or more cumbersome actually.
If we go for a hierarchy, I would suggest to rename "compactCommands" to something slightly more general because other compression mechanism might not fit in this scheme (and thinking about it, we don't really compact commands but events, right?)

bwaidelich · 2023-08-10T17:20:58Z

...r/Features/WorkspacePublishing/WorkspacePublishingWithExperimentalCommandCompression.feature

+      | text | "content 3" |
+
+    # THIS IS THE MODIFICATION
+    Then I expect exactly 8 events to be published on stream "ContentStream:cs-identifier"


clever way to test this and great job with the comments!

bwaidelich · 2023-08-10T17:24:14Z

Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php

@@ -506,6 +510,7 @@ private function handlePublishIndividualNodesFromWorkspace(
        );

        $originalCommands = $this->extractCommandsFromContentStreamMetadata($workspaceContentStreamName);
+        $originalCommands = $this->compactCommands($originalCommands);


Just a minor issue of course, but I would suggest to move the "feature flag" check out here, e.g.:

Suggested change

$originalCommands = $this->compactCommands($originalCommands);

if ($this->experiments->compactCommands_compressSimple()) {

$originalCommands = $this->compactCommands($originalCommands);

}

bwaidelich · 2023-08-10T17:24:32Z

Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php

@@ -627,6 +632,7 @@ private function handleDiscardIndividualNodesFromWorkspace(
        );

        $originalCommands = $this->extractCommandsFromContentStreamMetadata($workspaceContentStreamName);
+        $originalCommands = $this->compactCommands($originalCommands);


bwaidelich · 2023-08-10T17:26:51Z

Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php

+        if ($this->experiments->compactCommands_compressSimple()) {
+            $compressedCommands = [];
+            /* @var $lastSetSerializedNodePropertiesCommand \Neos\ContentRepository\Core\Feature\NodeModification\Command\SetSerializedNodeProperties */
+            $lastSetSerializedNodePropertiesCommand = null;
+            foreach ($commands as $command) {
+                if (
+                    $command instanceof SetSerializedNodeProperties
+                    && $lastSetSerializedNodePropertiesCommand !== null
+                    && $lastSetSerializedNodePropertiesCommand->appliesToSameNode($command)
+                ) {
+                    // COMPATIBLE: we can merge with a previous command, and reduce the command count.
+                    $lastSetSerializedNodePropertiesCommand = $lastSetSerializedNodePropertiesCommand->mergeWith($command);
+                } elseif ($command instanceof SetSerializedNodeProperties) {
+                    // Incompatible SetSerializedNodeProperties command => store the
+                    // last compressed command if any
+                    if ($lastSetSerializedNodePropertiesCommand !== null) {
+                        $compressedCommands[] = $lastSetSerializedNodePropertiesCommand;
+                        $lastSetSerializedNodePropertiesCommand = null;
+                    }
+
+                    $lastSetSerializedNodePropertiesCommand = $command;
+                } else {
+                    // different command type
+                    // so store the compacted command
+                    if ($lastSetSerializedNodePropertiesCommand !== null) {
+                        $compressedCommands[] = $lastSetSerializedNodePropertiesCommand;
+                        $lastSetSerializedNodePropertiesCommand = null;
+                    }
+
+                    $compressedCommands[] = $command;
+                }
+            }
+
+            // we are not allowed to lose commands
+            if ($lastSetSerializedNodePropertiesCommand !== null) {
+                $compressedCommands[] = $lastSetSerializedNodePropertiesCommand;
+                $lastSetSerializedNodePropertiesCommand = null;
+            }
+
+            return $compressedCommands;
+        } else {
+            return $commands;
+        }


as a result to the above I would suggest to remove the feature flag check here:

Suggested change

if ($this->experiments->compactCommands_compressSimple()) {

$compressedCommands = [];

/* @var $lastSetSerializedNodePropertiesCommand \Neos\ContentRepository\Core\Feature\NodeModification\Command\SetSerializedNodeProperties */

$lastSetSerializedNodePropertiesCommand = null;

foreach ($commands as $command) {

if (

$command instanceof SetSerializedNodeProperties

&& $lastSetSerializedNodePropertiesCommand !== null

&& $lastSetSerializedNodePropertiesCommand->appliesToSameNode($command)

) {

// COMPATIBLE: we can merge with a previous command, and reduce the command count.

$lastSetSerializedNodePropertiesCommand = $lastSetSerializedNodePropertiesCommand->mergeWith($command);

} elseif ($command instanceof SetSerializedNodeProperties) {

// Incompatible SetSerializedNodeProperties command => store the

// last compressed command if any

if ($lastSetSerializedNodePropertiesCommand !== null) {

$compressedCommands[] = $lastSetSerializedNodePropertiesCommand;

$lastSetSerializedNodePropertiesCommand = null;

}

$lastSetSerializedNodePropertiesCommand = $command;

} else {

// different command type

// so store the compacted command

if ($lastSetSerializedNodePropertiesCommand !== null) {

$compressedCommands[] = $lastSetSerializedNodePropertiesCommand;

$lastSetSerializedNodePropertiesCommand = null;

}

$compressedCommands[] = $command;

}

}

// we are not allowed to lose commands

if ($lastSetSerializedNodePropertiesCommand !== null) {

$compressedCommands[] = $lastSetSerializedNodePropertiesCommand;

$lastSetSerializedNodePropertiesCommand = null;

}

return $compressedCommands;

} else {

return $commands;

}

$compressedCommands = [];

/* @var $lastSetSerializedNodePropertiesCommand \Neos\ContentRepository\Core\Feature\NodeModification\Command\SetSerializedNodeProperties */

$lastSetSerializedNodePropertiesCommand = null;

foreach ($commands as $command) {

if (

$command instanceof SetSerializedNodeProperties

&& $lastSetSerializedNodePropertiesCommand !== null

&& $lastSetSerializedNodePropertiesCommand->appliesToSameNode($command)

) {

// COMPATIBLE: we can merge with a previous command, and reduce the command count.

$lastSetSerializedNodePropertiesCommand = $lastSetSerializedNodePropertiesCommand->mergeWith($command);

} elseif ($command instanceof SetSerializedNodeProperties) {

// Incompatible SetSerializedNodeProperties command => store the

// last compressed command if any

if ($lastSetSerializedNodePropertiesCommand !== null) {

$compressedCommands[] = $lastSetSerializedNodePropertiesCommand;

$lastSetSerializedNodePropertiesCommand = null;

}

$lastSetSerializedNodePropertiesCommand = $command;

} else {

// different command type

// so store the compacted command

if ($lastSetSerializedNodePropertiesCommand !== null) {

$compressedCommands[] = $lastSetSerializedNodePropertiesCommand;

$lastSetSerializedNodePropertiesCommand = null;

}

$compressedCommands[] = $command;

}

}

// we are not allowed to lose commands

if ($lastSetSerializedNodePropertiesCommand !== null) {

$compressedCommands[] = $lastSetSerializedNodePropertiesCommand;

$lastSetSerializedNodePropertiesCommand = null;

}

return $compressedCommands;

bwaidelich · 2023-08-10T17:27:52Z

Neos.ContentRepository.Core/Classes/SharedModel/Experimental/ContentRepositoryExperiments.php

+     * @param array<string,mixed> $in
+     * @api
+     */
+    public static function fromArray(array $in): self


$in is an unusual array name :)

bwaidelich · 2024-04-28T10:35:30Z

one more reservation towards this approach:
When trying to visualize the events of a publishing process I realized that we are missing a way to correlate published events to their original representation in the published content stream (see #3887 (comment)).
By merging commits that possibility will be gone

mhsdesign · 2024-11-02T20:54:45Z

With the new publishing v3 - as publish workspace behaves like publish individual nodes - i could imagine having this as a core feature. We already lowered the update interval of ckeditor neos/neos-ui#3751

The debounce timer has been extended from 500ms to 1500ms

though it might be sensible to apply both solutions?

And regarding the mixed up correlation to the original events ... maybe we should just compact commands that are directly send after another.

skurfuerst added the 9.0 label May 5, 2023

skurfuerst added this to the 9.0 (ES CR) milestone May 5, 2023

bwaidelich mentioned this pull request May 24, 2023

BUGFIX: do not crash when node already disabled/enabled #4284

Merged

bwaidelich mentioned this pull request Jul 7, 2023

Reduce number of NodePropertiesWereSet events during inline editing #4387

Closed

skurfuerst added 4 commits July 16, 2023 21:23

Merge remote-tracking branch 'origin/9.0' into 90-command-compression

db04cf6

FEATURE: Content Repository Experimentsf for trying out highly experi…

e8eac46

…mental stuff

Merge remote-tracking branch 'origin/9.0' into 90-command-compression

f52d738

FEATURE: Testcases for experimental command compression

f7072ee

skurfuerst marked this pull request as ready for review August 3, 2023 18:52

skurfuerst changed the title ~~FEATURE (performance): compress SetSerializedNodeProperties together~~ FEATURE (performance): compress SetSerializedNodeProperties together; allow experiments in codebase Aug 3, 2023

fix phpcs / phpstan

b741db9

bwaidelich reviewed Aug 10, 2023

View reviewed changes

mhsdesign marked this pull request as draft November 26, 2024 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE (performance): compress SetSerializedNodeProperties together; allow experiments in codebase #4263

FEATURE (performance): compress SetSerializedNodeProperties together; allow experiments in codebase #4263

skurfuerst commented May 5, 2023 •

edited by bwaidelich

Loading

bwaidelich commented May 24, 2023

skurfuerst commented May 24, 2023

bwaidelich left a comment

bwaidelich Aug 10, 2023

bwaidelich Aug 10, 2023

bwaidelich Aug 10, 2023

bwaidelich Aug 10, 2023

bwaidelich Aug 10, 2023

bwaidelich commented Apr 28, 2024

mhsdesign commented Nov 2, 2024

FEATURE (performance): compress SetSerializedNodeProperties together; allow experiments in codebase #4263

Are you sure you want to change the base?

FEATURE (performance): compress SetSerializedNodeProperties together; allow experiments in codebase #4263

Conversation

skurfuerst commented May 5, 2023 • edited by bwaidelich Loading

bwaidelich commented May 24, 2023

skurfuerst commented May 24, 2023

bwaidelich left a comment

Choose a reason for hiding this comment

bwaidelich Aug 10, 2023

Choose a reason for hiding this comment

bwaidelich Aug 10, 2023

Choose a reason for hiding this comment

bwaidelich Aug 10, 2023

Choose a reason for hiding this comment

bwaidelich Aug 10, 2023

Choose a reason for hiding this comment

bwaidelich Aug 10, 2023

Choose a reason for hiding this comment

bwaidelich commented Apr 28, 2024

mhsdesign commented Nov 2, 2024

skurfuerst commented May 5, 2023 •

edited by bwaidelich

Loading