-
Notifications
You must be signed in to change notification settings - Fork 12
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
Possible Performance Issues #134
Comments
My first step is going to be to try and reproduce this, even if un-scientifically. I plan to do a standard query, such as query for a list of posts. Capture the baseline with just WPGraphQL active. Then capture with just one plugin active, then the other, then both. See if we can at least kind of reproduce this. |
Reproduction:Just WPGraphQL
{
posts {
nodes {
id
title
date
content
}
}
} I executed this 7 times and here's the results:
WPGraphQL + WPGraphQL GutenbergWith WPGraphQL and WPGraphQL for Gutenberg (v0.4.1) active, I first indexed the Gutenberg Blocks from the WPGraphQL for Gutenberg settings:
Then I executed the same posts query as above (not querying for blocks at all) WPGraphQL + WPGraphQL Content BlocksWith WPGraphQL and WPGraphQL Content Blocks being the only active plugins:
WPGraphQL + WPGraphQL Content Blocks + WPGraphQL GutenbergWith all 3 plugins active, and still executing the same query for posts (not querying blocks)
It does seem that there's a perf hit with either WPGraphQL Gutenberg or WPGraphQL Content Blocks, and even more when running both together. |
Thank you, @jasonbahl, while I doubt this will be a quick fix it is absolutely something we'll be looking at as soon as we can get to it. |
Created a ticket to track this in our internal backlog. For internal users, it's available here: |
Thanks, Blake. We'll get that refined and, at the latest, in our next Sprint. |
We might need to add a performance testing pipeline to keep track of this. I suspect the reason behind this is calling WPGraphQL filters on interfaces which they might impact the code with hidden complexity. |
Been looking into this for WPGraphQL and my own work. I suggest taking a look at this repo, which is building off the work by the core performance team: https://github.com/swissspidy/compare-wp-performance
My own money is on the use of |
Ya, this is likely. My current thinking is that we can maybe disable the Would be good to "prove" that this is the culprit with a reproduction unassociated with the direct If we're able to confirm this is the culprit, then I think we should explore options for allowing types to eagerlyLoad for introspection (GraphiQL and other tooling needs) but not necessarily for run-time of queries. 🤔 |
We could also try to remove any calls to especially with the Anchor support: wp-graphql-content-blocks/includes/Field/BlockSupports/Anchor.php Lines 51 to 52 in 7789be5
|
WPGraphQL + WPGraphQL Content Blocks activeWith just WPGraphQL and WPGraphQL active, if I modify the code in WPGraphQL Content Blocks and change all instances of
This is promising. The tradeoff, however, is that individual Blocks are no longer visible in GraphiQL IDE. This is why I believe we might be able to detect if a query is an introspection query and if This would allow introspection queries for tools like GraphiQL to get the Types that aren't directly resolved via a field (individual Block objects, for example) while keeping run-time performance for other queries higher. Not 💯 sure this would work without other negative side-effects, but worth exploring I think. Another thing I think we could explore is allowing fields on types to be callbacks themselves. i.e. instead of registering a Type like so: register_graphql_object_type( 'MyType', [
'description' => __( 'My Type description that will be translated', 'text-domain' ),
'fields' => [
'fieldOne' => [
'type' => 'String',
'description' => __( 'Another thing that will be translated', 'wp-graphql' ),
],
],
] ); We might be able to allow types to be registered like so: register_graphql_object_type( 'MyType',
// Instead of a config array, we return a callback that will execute when the Type is instantiated
function() {
return [
'description' => __( 'My Type description that will be translated', 'text-domain' ),
'fields' => [
// Instead of a config array we provide a callback that will execute if/when the specific field is used in a request
'fieldOne' => function() {
return [
'type' => 'String',
'description' => __( 'Another thing that will be translated', 'wp-graphql' ),
];
},
]
];
}
); This proposal most likely would require some updates to core WPGraphQL, but would likely provide significant improvements as it would lead to a lot of code executing only when actually needed. Also, this is just a theory, but I believe it would have impact and might help with other things like internationalization that have reported other similar performance issues. |
possibly related: wp-graphql/wp-graphql#2721 |
@jasonbahl an alternative to wp-graphql/wp-graphql#2598 may possibly be cache some of the steps in a transient so it doesn't need to be rebuilt on every request ... I cant find an existing ticket, but I remember a recent discussion about it on Slack. |
@justlevine caching is easy. Cache invalidation is the hard part. What would the cache invalidation strategy of the Schema look like? The Schema could change via direct code calls to hooks/filters/register_graphql_* functions, post types being registered in code or via CPT UI etc, fields added by plugins like ACF / ACM / MetaBox.io, plugins like Pods, etc. If someone has really thought through how to go about caching the schema and more importantly invalidating the Schema, I'm interested in entertaining it, but I'm not seeing a path here. It would be like trying to cache the callbacks of "do_action" or "apply_filters". I think the real problem, or at least my theory of it, is that we're causing a lot more things to execute than needed. For example, field and Type descriptions, etc. Those are only actually needed for Introspection, but I believe we're still executing the translation functions even when the field(s) aren't needed for any given query. |
@jasonbahl agree with the concerns, but those are all engineering problems. Don't want to get too much into the weeds here for something that's solely relevant upstream, but essentially
💯. Caching is an additive strategy for the same goal of preventing unnecessary calls in the first place . |
Hey all, there doesn't seem to be anything actionable at the moment but we are tracking this conversation as this is important to us. |
@josephfusco as mentioned here
Currently we know that there is performance degradation when activating this plugin. The action now is to identify if it's something this plugin is doing specifically or not. The action is to see if any plugin that uses |
Or even a regular ol PHP profile run to see if any of the |
Clarification: we have an issue in the next sprint (starts next week) to look at this. We can test the ideas here and report our findings. |
@theodesp yes, this makes sense to me. the intent of For example, a plugin that wants to add an interface to the core In this case, since WPGraphQL Content Blocks is the same codebase registering blocks and the interface, it makes more sense (to me at least) to define the interfaces the block applies at the time the block itself is being registered |
Did some quick tests of vanilla site, using same query above.
|
Will create a jira ticket for this. |
Is this performance issue still noticeable? I am hoping to chart a migration path away from WPGraphQL Gutenberg but I will need both plugins activated as I cutover each of my queries. |
Hey @jeromecovington. I did some Load testing in a ticket last time. There are some performance improvements since the last update at about 15% more requests/s processed. There is a performance penalty (about 15%) when we use the If we set this to false here So when we do that we get back 15% more requests/s processed. However we do not see the GraphQL types in the documentation explorer. This is a drawback and I think this is an improvement that needs to be done from WPGraphQL side. In general terms the more blocks are available into the system then more work is needed to register and declare them. So if you have 300 blocks available then every time a request comes it has to go through the process. Also AFAIK having both WPGraphQL Gutenberg and Content Blocks are are doing the work twice so you are registering the same blocks and block attributes two times for each block thus you may see performance issues. |
It was reported in Slack that having WPGraphQL Content Blocks active at the same time as WPGraphQL Gutenberg leads to performance problems.
Steps to reproduce:
See degradation in server performance.
Slack conversation here for further context: https://wp-graphql.slack.com/archives/C3NM1M291/p1689789762004369
The text was updated successfully, but these errors were encountered: