8000 [Cache] Improve packing tags into items by nicolas-grekas · Pull Request #1 · sbelyshkin/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Cache] Improve packing tags into items #1

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.

Sign up for GitHub

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

Jump to bottom
Merged
nicolas-grekas merged 1 commit into from
Mar 23, 2022
Merged

[Cache] Improve packing tags into items #1

nicolas-grekas merged 1 commit into from
Mar 23, 2022

Conversation

@nicolas-grekas
Copy link
@nicolas-grekas nicolas-grekas commented Mar 21, 2022 •
edited
Loading

I think the attached patch would be a nice improvement on top of symfony#42997

Please don't merge, that's for discussion (Tests don't pass)

Basically, this delegate the serialization of tags to the wrapper adapters instead of letting TagAwareAdapter manage that part.

@nicolas-grekas nicolas-grekas force-pushed the ephemeral-tag-aware-adapters branch 3 times, most recently from ca8e161 to b2ef579 Compare March 21, 2022 18:27
nicolas-grekas
nicolas-grekas commented Mar 21, 2022
View reviewed changes
src/Symfony/Component/Cache/Adapter/TagAwareAdapter.php
$isTagPoolCleared = $this->tags->clear(static::TAG_PREFIX.$prefix);
} else {
$isTagPoolCleared = $this->tags->clear();
}
Copy link
Author
@nicolas-grekas nicolas-grekas Mar 21, 2022

Choose a reason for hiding this comment

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

Clearing tags doesn't make sense, to me. Let me know if you think I'm wrong.

Copy link
Author
@nicolas-grekas nicolas-grekas Mar 23, 2022

Choose a reason for hiding this comment

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

Second read: not clearing tags was on purpose. Tags could be shared among several item pools and clearing them for one pool would break this use case. If one wants to clear tags, they should call the method on the tags pool directly.

src/Symfony/Component/Cache/Adapter/TagAwareAdapter.php
$this->tags instanceof PruneableInterface && $this->tags->prune();

return $this->tags instanceof PruneableInterface && $this->tags->prune() && $isPruned;
return $this->pool instanceof PruneableInterface && $this->pool->prune();
Copy link
Author
@nicolas-grekas nicolas-grekas Mar 21, 2022

Choose a reason for hiding this comment

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

The return value only needs confirmation that the data has been pruned to me. Pruning tags is a bonus that doesn't impact the validity of pruning itself. Do you agree?

Copy link
Author
@nicolas-grekas nicolas-grekas Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, tags are persisted "forever", so they don't need to/cannot be pruned.

@nicolas-grekas nicolas-grekas force-pushed the ephemeral-tag-aware-adapters branch 3 times, most recently from 014765e to f34d82a Compare March 23, 2022 10:53
@nicolas-grekas
Copy link
Author
nicolas-grekas commented Mar 23, 2022

PR updated to keep compat with already populated pools.
I still need to fix tests and to have a closer look at your version of getTagVersions.

@nicolas-grekas nicolas-grekas force-pushed the ephemeral-tag-aware-adapters branch 2 times, most recently from c59be36 to ec21011 Compare March 23, 2022 18:02
@nicolas-grekas
Copy link
Author
nicolas-grekas commented Mar 23, 2022 •
edited
Loading

Now with a special storage class (named Ͼ for making it as short as possible).
This could also act as a DTO between adapters and items, to store metadata more efficiently (eg using an HSET on redis).
This is left as an exercise for another iteration :)

@nicolas-grekas nicolas-grekas force-pushed the ephemeral-tag-aware-adapters branch from ec21011 to e252a3c Compare March 23, 2022 18:32
@nicolas-grekas
Copy link
Author
nicolas-grekas commented Mar 23, 2022

I added back MAX_NUMBER_OF_KNOWN_TAG_VERSIONS, with an LRU-based implementation.

Let me know if I missed any features/behaviors that you wanted to achieve @sbelyshkin

I'm going to wait for your feedback/review now.

@nicolas-grekas nicolas-grekas mentioned this pull request Mar 23, 2022
Merged
@nicolas-grekas nicolas-grekas force-pushed the ephemeral-tag-aware-adapters branch from e252a3c to 210b4cb Compare March 23, 2022 19:01
@nicolas-grekas
Copy link
Author
nicolas-grekas commented Mar 23, 2022 •
edited
Loading

Last change: added $persistTags on getTagVersions(), to not create tags when only fetching them for comparison.

@nicolas-grekas nicolas-grekas merged commit 210b4cb into sbelyshkin:ephemeral-tag-aware-adapters Mar 23, 2022
sbelyshkin
sbelyshkin reviewed Mar 24, 2022
View reviewed changes
src/Symfony/Component/Cache/Adapter/ArrayAdapter.php

private bool $storeSerialized;
private array $values = [];
private array $tags = [];
Copy link
Owner
@sbelyshkin sbelyshkin Mar 24, 2022

Choose a reason for hiding this comment

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

Should regular adapter know about tags?

Copy link
Author
@nicolas-grekas nicolas-grekas Mar 24, 2022

Choose a reason for hiding this comment

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

Only about metadata, and tags are part of them. That's OK to me.

sbelyshkin
sbelyshkin reviewed Mar 24, 2022
View reviewed changes
sbelyshkin
sbelyshkin reviewed Mar 24, 2022
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbelyshkin sbelyshkin sbelyshkin left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nicolas-grekas @sbelyshkin
0