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

FEATURE: improve performance for publish / discard workspaces #4286

Draft
wants to merge 2 commits into
base: 9.0
Choose a base branch
from

Conversation

skurfuerst
Copy link
Member

@skurfuerst skurfuerst commented May 16, 2023

neos ui part: neos/neos-ui#3494

... this ensures that during publishing of nodes, we do not fork sub-processes for event processing (for every event). This speeds up the system usually by at least factor 2-3 during these operations.

It could have some side-effects if caches were not properly cleared, but at least I did not see any yet (and we run tests often with this flag).

Resolves: #4285
Related: #5303

... this ensures that during publishing of nodes, we do not fork
sub-processes for event processing (for every event). This speeds
up the system usually by at least factor 2-3 during these operations.

It *could* have some side-effects if caches were not properly cleared,
but at least I did not see any yet (and we run tests often with this flag).

Resolves: #4285
@skurfuerst skurfuerst added the 9.0 label May 16, 2023
@skurfuerst skurfuerst self-assigned this May 16, 2023
@skurfuerst skurfuerst added this to the 9.0 (ES CR) milestone May 16, 2023
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

I have to admit that I struggle a bit with this direction as it turns this somewhat hacky global state to "user land" and I can already see lots of places where this will sneak in to optimize performance eventually turning big parts of the system into a single-process which makes it more fragile I'm afraid.

I'm currently experimenting with a greatly improved catchup mechanism that can be interacted with via websockets and I hope that this will speed up performance drastically.

@skurfuerst are you OK with waiting a bit longer to see if my approach actually works out?

@skurfuerst
Copy link
Member Author

@bwaidelich sure :) go ahead :):)

@mhsdesign
Copy link
Member

I agree with @bwaidelich and share the concern that the synchronous option will sneak as "performance improvement" into client code and other packages as well.

Currently speeding up the catchup is a big concern, as we block nearly EVERY command we issue. That is mostly caused because we need to update the projections so our soft constraints work (but bastian told me about an alternative way: in memory content graph projection, maybe you can elaborate further @bwaidelich )

@bwaidelich
Copy link
Member

but bastian told me about an alternative way: in memory content graph projection, maybe you can elaborate further @bwaidelich

I'll try (:
an in memory content graph is actually not a solution for the constraint checks (at least not a fully in-memory implementation) because we would have to load every event of a given content stream to rebuild it everytime there is a new event :)
But there are two possible solutions for avoiding to have to rely on the read model to check the constraints for a future version

a) Graph for the write side

A simplified version of the graph that is only concerned about the hierarchy (not about properties etc) that is kept up to date.
For every new command it will apply the corresponding changes to that graph in a "rollback only" transaction if that fails, constraints failed. Otherwise the event is published (and the changes are applied for good).

I tested this in a PoC with ~20k pending changes in one transaction and it seemed to work fine.

b) Dynamic Consistency Boundary

a new concept (long post!) that would basically allow build up a tree like

root
  a (version: 2)
    a1 (version: 2)
    a2 (version: 3)
  b (version: 4)
    b1 (version: 5)
    b2 (version: 5)

and then do constraint checks only in the affected subtree.
E.g. when I move a node underneath a1 in that example, I can check violations (parent node exists, node is not moved into a descendant node of itself, ...)
and finally commit the new events by specifying all affected ascendant nodes (a1, a, root) and the highest version (2) and the event would go through unless a new event matching that subtree was published in the meantime.

Probably this wasn't really clear. But the main benefit compared to a) is that you could move and change different parts of the tree at the same time without risking race conditions (leading to failing commands)

That's all for the future though. For 9.0 I would like to tweak the catch up handling at least:
I'm working on some coordinator that makes sure that one catchup is never triggered more than once at the same time.
With a backround worker (and websockets to communicate) this is already really fast but it will also improve the performance when used without a worker script.
My PoC is currently being tested in a real-life project, I hope that I can release something soon

@bwaidelich
Copy link
Member

btw: the InMemory-Graph I mentioned for a potential "compraction" mechanism: With it we could rebuild a graph at any version and create the minimal amounts of events from the result.
An alternative could be a persistent data structure – that keeps all versions alive at any time.. Very intriguing but probably not feasible – at least for the CMS case

@bwaidelich
Copy link
Member

@skurfuerst I think this one is obsolete with our in-process (sync) CR, right?

@mhsdesign mhsdesign marked this pull request as draft October 29, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance for publish/discard of workspaces by using the synchronous mode
3 participants