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

[Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter #33939

Merged
merged 1 commit into from Oct 11, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Oct 9, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets #33924
License MIT
Doc PR -

This is the final touch in my series of PR that fixes the linked issue.

Remarkably, the solutions I implemented for this issue are completely different than the one I described there. Fortunately, the issues themselves were correctly identified.

Plannification of implementation is gambling :)

/cc @andrerom

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

What does this actually do (is there now double serialization of value for instance?), and why does it do it?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

  • right now, the data is stored as: serialize([$value, $tags])
  • this PR stores it as serialize([serialize($value), serialize($tags))

This allows unserializing only the tags when only them are needed, in doDelete especially. The value is still downloaded but not unserialized anymore in this method.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from b87f933 to 9d760fb Oct 10, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from 9d760fb to dbda21b Oct 10, 2019
@andrerom

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

this PR stores it as serialize([serialize($value), serialize($tags))

ok, that's what I thought. Anyway we can avoid the double serialization?
As in, could be two chunks of serialized blobs with a predictable separator between them for instance.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

As in, could be two chunks of serialized blobs with a predictable separator between them for instance.

I suppose yes, I'll have a look.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from dbda21b to bbf540b Oct 10, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

@andrerom updated, please have a look at TagAwareMarshaller.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch 2 times, most recently from a60d466 to b2269ca Oct 10, 2019
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from b2269ca to 0cc9ff5 Oct 10, 2019
Copy link
Contributor

left a comment

Looks to add quite a bit of complexity. Sure you don't want to solve this by just storing tags separately in adapters where that makes sense and for now treat indeed serialization on delete as know issue for file system? (until we have a reliable way to do parallel and/or async file reading)

Anyway, added several review comments here, some might be missing the point though.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from 0cc9ff5 to cc35a60 Oct 10, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

@andrerom comments addressed hopefully.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from cc35a60 to 2f7f91e Oct 10, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

@andrerom

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2019

E.g. we'd want values and tags to be stored on the same Redis node

That would be the simple part I guess using key like {cache-key}-tags or similar.

first needed step that fixes the unserialization issue in a generic way.

ok 👍 If we find time afterwards we can see if we can approach this using non blocking streams for Filesystem or something to contemplate splitting tags in separate files.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2019

Actually for filesystem another approach would work better: we read the first bytes of the file and stop before reading the value. The data format provided by this PR allows that.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch 3 times, most recently from 005b90b to 74296cd Oct 11, 2019
Copy link
Contributor

left a comment

+1 besides nitpick

src/Symfony/Component/Cache/Traits/RedisTrait.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from 74296cd to 0c32b84 Oct 11, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2019

Good news: I managed to remove over fetching for both Redis and Filesystem adapters! Once again, the solution is completely different than we originally anticipated :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch 3 times, most recently from b666d2e to b029f52 Oct 11, 2019
Copy link
Contributor

left a comment

Even better :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from b029f52 to 33f31ca Oct 11, 2019
@nicolas-grekas nicolas-grekas changed the title [Cache] serialize tags separately from values in AbstractTagAwareAdapter [Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter Oct 11, 2019
…stractTagAwareAdapter
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-late-unser branch from 33f31ca to 5a4a30c Oct 11, 2019
nicolas-grekas added a commit that referenced this pull request Oct 11, 2019
…e when using AbstractTagAwareAdapter (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #33924
| License       | MIT
| Doc PR        | -

This is the final touch in my series of PR that fixes the linked issue.

Remarkably, the solutions I implemented for this issue are completely different than the one I described there. Fortunately, the issues themselves were correctly identified.

Plannification of implementation is gambling :)

/cc @andrerom

Commits
-------

5a4a30c [Cache] add TagAwareMarshaller to optimize data storage when using AbstractTagAwareAdapter
@nicolas-grekas nicolas-grekas merged commit 5a4a30c into symfony:4.4 Oct 11, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:cache-late-unser branch Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.