Use post object in post date block

[spacedmonkey]

What?

In the post date block, this results in lots of calls to get_post as post_id is passed on many functions. Instead of using a id, get it once and reuse that.

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

[github-actions]

Flaky tests detected in 5e28160.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4158836622
📝 Reported issues:

• [Flaky Test] should handle Enter across blocks #39763 in specs/editor/various/multi-block-selection.test.js
• [Flaky Test] should insert block with the slash inserter when using multiple words #40301 in specs/editor/various/inserting-blocks.test.js
• [Flaky Test] should not allow blocks to be inserted in inner blocks #44341 in specs/editor/plugins/cpt-locking.test.js

[carolinan]

I don't know how to review this to confirm that it is a performance improvement, can you explain it in more detail?
The date functions still use get_post() internally. Only get_permalink checks if it is a post object.

[spacedmonkey]

I don't know how to review this to confirm that it is a performance improvement, can you explain it in more detail? The date functions still use get_post() internally. Only get_permalink checks if it is a post object.

@carolinan That is the point. By calling get_post in one place, it saves the cache lookup in the 5 places that need it. This stops a lot of code from running, like the cache lookup and creating of WP_Post object.

[spacedmonkey]

[carolinan]

Please consider that for someone who is unfamiliar with the tools you are using, the image has no context.

[spacedmonkey]

Please consider that for someone who is unfamiliar with the tools you are using, the image has no context.

What information do you need here? I have provided detail of why this is faster and provided a screenshot of why that is the case.

[ghost]

@spacedmonkey Thanks for the pull request, I believe you're on to something here. The issue is that you're leaving out a lot of context (starting with not giving any info beneath the "Why?", "How?" and "Testing Instructions" sections which are there for a reason) which makes it hard to verify the issue for reviewers.

In the post date block, this results in lots of calls to get_post as post_id is passed on many functions. Instead of using a id, get it once and reuse that.

What is "this"? How many is "lots of calls"? What functions are "many functions"? Not trying to be annoying here, but this info you're now leaving out is what would paint a complete picture.

@carolinan That is the point. By calling get_post in one place, it saves the cache lookup in the 5 places that need it. This stops a lot of code from running, like the cache lookup and creating of WP_Post object.

Can you specify the 5 places that are now calling get_post when they don't have to? And specify "a lot of code"?

I have provided detail of why this is faster and provided a screenshot of why that is the case.

As @carolinan pointed out, and I can confirm, the screenshot provided is not providing any additional information to anyone not familiar with the tool you've used. While it's undoubtedly clear to you, personally I have no idea what information the screenshot should convey. E.g. what do the percentages mean?

Anyone can dig into the code and probably find out themselves, but that's not doable for every single pull request. I hope you understand and are willing to provide some more context and information.

[carolinan]

@spacedmonkey I trust you. But I can not do a code review if I do not fully understand it.
Of course, I am not attempting to block anything. Someone else will be capable of reviewing it.

[spacedmonkey]

@thomasdevisser @carolinan

As I have said, not sure what more context I can give. But I will try. This change has not functional changes in it and will not see anything unless you run a php profiler against it. This is not to test other than run a your favourite profiling tool against.

Why is this code better than before. Simple, instead of passing an id, and running get_post 5 times in each function, do it once. This results in 5 less cache lookups. This makes things faster and is less wasteful. Simple.

[ghost]

Like @carolinan said get_post still gets called, even if you already give it the post object instead of just a post ID. It has no check to see if it's an ID or post object, so it doesn't reduce the amount of calls to get_post. What your change does achieve, is change what happens inside the get_post function.

Here you no longer need to get the instance, which would indeed result in better performance.

[spacedmonkey]

Like @carolinan said get_post still gets called, even if you already give it the post object instead of just a post ID. It has no check to see if it's an ID or post object, so it doesn't reduce the amount of calls to get_post. What your change does achieve, is change what happens inside the get_post function.

It does not reduce the number of calls to get_post, but lookups to cache and creating on new post objects. If you give it a object and doesnt need to lookup by id and create a new post.

[spacedmonkey]

@joemcgill @felixarntz Can you take a look at this PR? This is a very simple, instead of creating 4 new post objects, create one and pass it in. Quick and easy.

[felixarntz]

@spacedmonkey The change looks good and should certainly improve performance, in theory. Though from looking at it, I would think this is a micro optimization - which doesn't mean that we shouldn't do it, but there would be 1000s of similar fixes possible in core, and then the question is how valuable is it to tackle all of them vs focusing on larger areas of improvement.

Can you share what % of overall processing time this improves e.g. on a page that has 1 or 2 core/post-date blocks? How about a blog archive that has 20 core/post-date blocks? I think depending on those numbers it will be easier to see how large or small the gains here are.

[spacedmonkey]

@spacedmonkey The change looks good and should certainly improve performance, in theory. Though from looking at it, I would think this is a micro optimization - which doesn't mean that we shouldn't do it, but there would be 1000s of similar fixes possible in core, and then the question is how valuable is it to tackle all of them vs focusing on larger areas of improvement.

Can you share what % of overall processing time this improves e.g. on a page that has 1 or 2 core/post-date blocks? How about a blog archive that has 20 core/post-date blocks? I think depending on those numbers it will be easier to see how large or small the gains here are.

This is a micro optimization and does not require this much thought or review. Just found this while working on other things and decided to fix it. There are likely MANY MANY more like this. But let's do this a lots of PRs, as much it easier to review and test.

[joemcgill]

This change looks fine to me, but I agree with @felixarntz that we should evaluate the actual performance benefit in terms of wall time/memory saved by this type of change before refactoring all the places that functions in WP can accept a post ID and a post object where a post ID is being passed.

[felixarntz]

@spacedmonkey

This is a micro optimization and does not require this much thought or review.

It is a simple change, but like I mentioned we could make 1000s of those changes. The question is whether it's worth it. Every change still has at least a tiny chance for breakage. And there's also the aspect of our time investment into those little fixes - is this type of micro optimization worth changing as many instances in core as we can to pass objects rather than IDs?

I think we should still make an assessment of how beneficial this truly is before going down that rabbit hole.

[ghost]

@felixarntz I understand everything you're saying, and I fully agree that it might not be beneficial to do the 1000 PRs; but it feels like you're saying it's either/or. Either we do all 1000 or nothing. Why not just merge this one and if someone finds another, we merge that one as well? Without actively looking for every single occurrence.

[ghost]

We could and maybe should of course still do an assessment for wether or not we're gonna actively look for these kind of refactors over the entire codebase.

[felixarntz]

@tomdevisser I'm not saying we should do all or nothing. But I would still like us to get a better idea whether this brings any notable real improvement or is just more of a hypothetical one, for the reasons I mentioned above.

For example, if the outcome was that we would need to change a million instances of this call to get a 1ms load time improvement, it's probably just not worth working on this. But if the outcome was that this change brings a consistent 0.1ms or maybe even 0.01ms improvement, that's a whole different story, and then it would be more beneficial. It all comes down to: How long does it take to run get_post() 10, 100, 1000, 10000, ... times? And whether that cost is considerable or negligible.

[ghost]

it's probably just not worth working on this

@felixarntz I hear you, really. It just feels like discussing this has already taken time in which we could've merged this code (that's already been reviewed and ready to merge) 10 times. 😬 I agree on almost everything you're saying, just not the "worth it" and "takes time" argument.

Might be naive as a new contributor, but even if this changes the performance by 0.00001%, merging it would've taken less time than this discussion. 😅 I feel like this going back and forth isn't worth it for this issue, although it would be for a bigger assessment.

[felixarntz]

@tomdevisser I'm not meaning to "discuss" this - I am simply asking to make an actual performance assessment about this change before we move forward with it.

[ghost]

@felixarntz All right. I would say, merge this one and create a new issue to do the assessment to decide whether or not to do this on a bigger scale instead of stalling this PR. But I'll leave it from here, cause I get the feeling you've made up your mind and are digging your heels in.

Good luck with the follow up.

[felixarntz]

@tomdevisser When you say "stalling this PR", it sounds like this performance analysis would take some time, which I don't think is the case. It just needs to be done, I'm sure it'll take less than 1 hour to assess the impact of this change and the cost of get_post() in general.

There is no rush in merging this. This won't make it into WordPress 6.2 (since it is too late in the release cycle for enhancements like this to be merged), and WordPress 6.3 is a few months away.

I personally believe we should assess changes before we commit them, not afterwards. There's also always a risk that otherwise such a follow up never happens.

[ghost]

@felixarntz Got it, thanks for specifying. Have never done one before so I have no idea how much work it would be. Sorry if it came across wrong, there's a language barrier (as I'm Dutch) that can be tough when there's a disagreement, but I'm trying. 🙂

[felixarntz]

@tomdevisser No worries at all! I'm German myself, so apologies if anything didn't come across right. 🙂

[spacedmonkey]

Closing this ticket, as this discussion is unproductive.

[felixarntz]

@spacedmonkey Could you clarify what you mean by "this discussion is unproductive"? All I'm asking for is that we assess this change in terms of its actual performance impact before just moving forward with it.

I'm not at all against this change, but I think we should get a better idea about whether this is a micro optimization or an optimization that is actually worth it. Most importantly, what is the cost of a get_post() call, so how much do we actually save here?

[ghost]

@felixarntz Hi Felix, I sent you a message on the MWP Slack about this measuring of performance, if you have some time I'd love to hear from you.

[ghost]

Reopening this and doing some performance checks.

First test, after performing the suggested approach by @felixarntz:

One approach would be to implement some quick PHP function in a WordPress plugin that e.g. calls get_post() 10, 100, 1000, 10000, ... times, and measure how long that takes (e.g. via microtime( true ))

string(55) "Ran get_post() 100000 times in 0.15558886528015 seconds"
string(55) "Ran get_post() 100000 times in 0.15703010559082 seconds"
string(55) "Ran get_post() 100000 times in 0.15356588363647 seconds"
string(55) "Ran get_post() 100000 times in 0.16105008125305 seconds"
string(54) "Ran get_post() 100000 times in 0.1589879989624 seconds"
string(55) "Ran get_post() 100000 times in 0.15635204315186 seconds"

@felixarntz

[Mamaduka]

@aristath, @oandregal, I know you've also been doing profiling for the PHP side of the block editor. Maybe you can point @tomdevisser in the right direction on how to profile this branch vs. trunk.

[ghost]

Also tested this suggested approach by @felixarntz.

Another approach would be to take a site and test the performance of the core/post-date block, with and without that PR branch added to the site. You could add a static variable to the code of the core/post-date block to measure the duration of every time it is called within a single page load and e.g. log the results in wp_footer.

Results before:

Results after:

So it looks to me like there's a slight regression in performance, but not sure if I'm reading the results correctly?

[felixarntz]

@tomdevisser Based on your data, I would say this looks neither like a notable regression nor like a notable improvement. There's a good chance that that's actually the case. But to rule that out, we may want to measure with a larger dataset. Performance metrics vary, even between testing the exact same thing multiple times, as this data confirms once again. To potentially counter that, we need to test far more often, let's say 100 times or even 1000 times.

Obviously that cannot reasonably be done manually, so we'd need to go with an automated approach. For example, use the Performance Lab plugin's Server-Timing API (see WordPress/performance#553) to capture the results in a response header and then use a script like https://github.com/GoogleChromeLabs/wpp-research/tree/main/cli#benchmark-server-timing to trigger requests, extract the header and compute percentiles/median.

Maybe we'll see a performance improvement with that - maybe not. If the latter, it would mean that the impact is extremely small. We can of course argue based on the code change and our experience as developers that this is a performance enhancement, which I definitely don't disagree with. But I would also personally not consider this a priority then, especially as it does open a can of worms with tons of similar code examples present in WordPress core.