- πΊπΈUnited States nicxvan
It's opt-in: a per-field configuration that defaults to false. So on each site you'll be able to choose which fields, if any, to apply it to.
That resolves our concerns!
I missed: https://git.drupalcode.org/project/drupal/-/merge_requests/9417/diffs#83...
- πΊπΈUnited States effulgentsia
xxHash looks good, Symfony uses the 128 bit version of xxHash in places, so let's follow suit here
I agree with 128 bit rather than 64 bit. https://en.wikipedia.org/wiki/Birthday_attack has a table that shows that with 64 bits, you'd have more than a 1 in a million chance of collision once your table grows above 6 million records.
However, instead of a 32 character hex string, we could reduce it to a 24 character base64 string.
- πΊπΈUnited States effulgentsia
it will help reduce database size with Paragraphs as well as Experience Builder
Just to set expectations, this might help some with Paragraphs but probably not as much as it will help with Experience Builder. For Paragraphs, it will help with individual fields that have long content (if those fields opt into it, per #19), such as
text_long
fields that content editors populate with a lot of text. However, for some sites, Paragraphs don't necessarily have a ton of content in any one field, they just have a lot of fields, and in those cases, the work in this issue won't help much with that.Because Experience Builder will combine values for all fields within a component into a single JSON field or Field Union field, this issue will help with the "lots of fields with small content" use case as well as the "single fields with large content" use case.
- πΊπΈUnited States effulgentsia
It's opt-in: a per-field configuration that defaults to false. So on each site you'll be able to choose which fields, if any, to apply it to.
For fields that opt into it, it adds an indexed JOIN to loading and querying revisions.
For fields that opt into it, it adds having to compute a hash for every field item for every translation when writing a revision (saving an entity). It's possible that this could be expensive (and we might want to consider future optimizations to track which items/translations are dirty and need re-hashing), but xxHash is very fast (so dirty tracking might not be needed).
- πΊπΈUnited States nicxvan
Is this trading runtime efficiency for storage efficiency?
I'm concerned this makes saving and loading fields slower to hash revisions for the majority of sites. It seems it will make queryng revisions more complex.
I have a client with 10 languages and many nodes. When I shared this with them they raised concerns about the performance trade off since cpu is more expensive than storage.
- π¬π§United Kingdom longwave UK
Some test fails around installed config, but while I sort those out and add new test coverage marking as NR to see if anyone has any early comments on the approach so far.
Will also need an upgrade path for existing config if we go this route.
- π¬π§United Kingdom longwave UK
Back to my thoughts in #8:
- While it makes sense to only do this for blob/text fields I don't think it makes sense to defer storage layout to the field type. I started prototyping this route but it turns out to be even more hacky. Discussed with @effulgentsia and while this probably only applies to core text fields, JSON fields in contrib and perhaps a small number of others, there is no benefit to it being a separate field type.
- @effulgentsia also reminded me that base fields currently apply to all bundles in an entity. While we could allow fields on the base table to be NULL for bundles that don't use the field, and I've never really understood why e.g a boolean field of cardinality 1 requires an entirely separate table for its storage, I think this requires more discussion and is out of scope here - if I add test coverage then at least we can refactor at will later if we decide to make field storage pluggable in some way.
- xxHash looks good, Symfony uses the 128 bit version of xxHash in places, so let's follow suit here.
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
We ran into this same bug while working on Experience Builder over at #3468106-26: Render component tree inside the default theme, as a regular page β .