-
-
Notifications
You must be signed in to change notification settings - Fork 541
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
[5.x] Augmentation performance improvements #9636
Merged
jasonvarga
merged 45 commits into
statamic:master
from
JohnathonKoster:deferred-all-augmentation
May 2, 2024
Merged
[5.x] Augmentation performance improvements #9636
jasonvarga
merged 45 commits into
statamic:master
from
JohnathonKoster:deferred-all-augmentation
May 2, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Positive improvements across a variety of different situations, particularly the `group_by` modifier
JohnathonKoster
changed the title
[5.x] Deferred all augmentation
[5.x] Defer all augmentation, allow for Bulk Augmentation (reuse key/fields across similar items), even more internal augmentation adjustments
Mar 2, 2024
* Allow for implementors to return `null`, which disables the bulk process for that item * Entry now takes data keys into consideration to prevent "locking" an entry into something if it has custom data
37 tasks
jasonvarga
previously requested changes
Mar 7, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please target master
.
# Conflicts: # src/Entries/Entry.php
…some just because ... - Dedicated AbstractAugmented 'wrap' methods, to avoid passing around hard to follow booleans. - Avoid a hard to follow boolean in InvokableValue - Make InvokableValue easier to follow both paths in general. - TransientValue and DeferredValue now pass their references when calling shallow(), which were missing, and was the reason for needing to check for nulls. - Typehints and visibility
… ... If you pass a closure as the raw value, it'll lazily evaluate them when resolving.
materialize() just called resolve() and returned a new Value instance. Now that the subclasses are gone, there's no need to new up a separate Value instance. It can just return itself. With that gone, we might as well just call resolve since materialize would just be an alias.
jasonvarga
changed the title
[5.x] Defer all augmentation, allow for Bulk Augmentation (reuse key/fields across similar items), even more internal augmentation adjustments
[5.x] Augmentation performance improvements
May 1, 2024
- Add static factory methods to BulkAugmentor. - Rename BulkAugmentor augmented() to toArray(). Seemed more inline with what it did. Looks like it makes sense when compared to the toDeferredAugmentedArray in the "else" where its used in RuntimeValues. - Reduce visibility where possible
Since dd(), dump(), etc can have variable arguments, avoid making our own Dumper methods. Just use the class to resolve the values and pass them into the original functions. Rename resolveValues to resolve. Simplify map. Remove unnecessary guard.
1 task
This was referenced May 14, 2024
This was referenced Jul 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an extension of #9574, and reworks some more internal augmentation behaviors. The changes in this PR come together to allow us to reuse fields arrays and augmentation keys as much as possible in a new
BulkAugmentor
. This new class will work to reduce the overhead of resolving this information as much as possible. For example, if you want to augment 1,000 entries and they can all safely reuse the same fields/keys, theBulkAugmentor
will only do those calls once. This PR only focuses on pages/entries for now.Value
objectsget
directly, they shouldn't need to care about materializationInvokableValue
andDeferredValue
even more lazy 🥳Rework collection tag to use transient valueswill just handle this Antlers-side to cause less chaos