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

Overhaul ContentCacheFlusher #5175

Closed
dlubitz opened this issue Jul 5, 2024 · 2 comments · Fixed by #5221
Closed

Overhaul ContentCacheFlusher #5175

dlubitz opened this issue Jul 5, 2024 · 2 comments · Fixed by #5221
Assignees
Labels

Comments

@dlubitz
Copy link
Contributor

dlubitz commented Jul 5, 2024

At the moment the ContentCacheFlusher can't flush caches discarded nodes. This seems to be caused by the fact, that after persisting the change (discard), the ContentCacheFlusher tries to identify his parents, which is not possible anymore.

To solve this issue, we need to do all node traversing operation BEFORE the persistence. Which should be done in the GraphProjectorCatchUpHookForCacheFlushing. So the ContentCacheFlusher does not need to do any node traversing anymore and simply flush all given caches.

Currently this doesn't have a effect, as the cacheEntryIdentifier is based on the contentStreamId, which leads to orphan cache entries after each new contentStream creation. On the other hand, the cache entry is not used anymore after discard, which means the same as a cache flush of the entry.

See also:

@dlubitz dlubitz added the 9.0 label Jul 5, 2024
@dlubitz dlubitz self-assigned this Aug 23, 2024
@dlubitz dlubitz moved this to In Progress 🚧 in Neos 9.0 Release Board Aug 23, 2024
dlubitz added a commit to dlubitz/neos-development-collection that referenced this issue Aug 23, 2024
dlubitz added a commit to dlubitz/neos-development-collection that referenced this issue Aug 26, 2024
dlubitz added a commit to dlubitz/neos-development-collection that referenced this issue Aug 26, 2024
dlubitz added a commit to dlubitz/neos-development-collection that referenced this issue Aug 27, 2024
dlubitz added a commit to dlubitz/neos-development-collection that referenced this issue Aug 27, 2024
dlubitz added a commit to dlubitz/neos-development-collection that referenced this issue Aug 27, 2024
@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✅ in Neos 9.0 Release Board Aug 30, 2024
@mhsdesign
Copy link
Member

mhsdesign commented Sep 25, 2024

I just have some followup question if i may ^^ so all our decisions are documented for the future :D

Now we have the capability to flush a whole workspace if we want which is really powerful.

1.) What was the reasoning against more fine grained flushing on WorkspaceWasDiscarded like only flushing all discarded nodes? Sure it would definitely require a dependency to the ChangeProjection as drawn out here but that would allow to keep parts of the cache entries and thus optimise performance right?

2.) Also i played around to optimise DiscardIndividualNodes by trying to flush $eventInstance->discardedNodes but as found out by wilhelm in slack we cannot fetch the node aggregate - not even in onBeforeEvent because the content stream is already long gone. Question is will that haunt us in the long term? (This is also manifested in the codebase

* NOTE: you can rely on the fact that an extra {@see ContentStreamWasForked} event was persisted BEFORE
)

I 've got "Discard All" working, but one problem remains: On DiscardIndividualNodes, the WorkspaceCommandHandler removes the old content stream before the WorkspaceWasPartiallyDiscarded event is emitted. This makes it impossible to flush the content cache for the affected nodes. I think it'd be safe to change the order of events there, but this would brake the convention used in that class. Any ideas/opinions?

  1. Should we also flush on workspace deletion (WorkspaceWasRemoved)?

Edit i might actually recall us saying that discard is not that important to optimise as it will happen not that often.... and maybe optimising rebase will rather be worth our brain power.

@dlubitz
Copy link
Contributor Author

dlubitz commented Oct 1, 2024

  1. The WorkspaceWasDiscarded event has no information about the discarded nodes. I don't know if it's worth to optimize this by making it more complex and also cache flushing itself is an expensive operation.

  2. Simplicity. We wanted to keep it easy. Also the performance is dependent on the amount of discarded node. If you need to discard more single nodes, this is more expensive than the workspace flush.

  3. Yeah, maybe. Didn't tought about this. But nothing urgent for the release.

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this issue Nov 30, 2024
Instead of fine granular updating only the usage references on the nodes that were discarded, well throw away everything and then rely on the reapplied events what is kept.

This is more consistent to how other parts react to publishing like the graph just forks a new content stream and doesnt care about the old data, the change projection behaves similar to the graph.
And for the content cache flusher we had the discussion to use `discardedNodes` but opted against it and do a full flush there as well: neos#5175 (comment)

This change allows us to get rid of the `discardedNodes` property (with dimensions) in the `WorkspaceWasPartiallyDiscarded` event.
neos-bot pushed a commit to neos/neos that referenced this issue Dec 12, 2024
Instead of fine granular updating only the usage references on the nodes that were discarded, well throw away everything and then rely on the reapplied events what is kept.

This is more consistent to how other parts react to publishing like the graph just forks a new content stream and doesnt care about the old data, the change projection behaves similar to the graph.
And for the content cache flusher we had the discussion to use `discardedNodes` but opted against it and do a full flush there as well: neos/neos-development-collection#5175 (comment)

This change allows us to get rid of the `discardedNodes` property (with dimensions) in the `WorkspaceWasPartiallyDiscarded` event.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants