do_blocks(): Free up transient memory leak

[dmsnell]

There has been a memory inefficiency inside of do_blocks() where it parses the given content then iterates through each top-level block to render it. Unfortunately each top-level block can considerably add to the overall memory use involved, and once moving on to the next top-level block, all of the allocated memory will be retained until the end of the call to do_blocks().

In this change, each parsed block sub-tree is freed via reset to null after it has been rendered. All top-level blocks are rendered independently of each other and so this operation is safe — there are no data dependencies between them.

For a test post of length 2.3 MB containing 138 top-level blocks, this brought the minimum memory requirement for the PHP process down from around 450 MB to around 316 MB. This was “around” since the memory requirements are non-deterministic and some runs will succeed while other runs will crash for out-of-memory given the same memory_limit.

Impact of this change will be most profound when there exist one or more top-level blocks which allocate a significant amount of memory which are not the last top-level-block in a post. This means that this change might even impact small and typical posts, if the right blocks are near the start of the post.

Trac ticket: Core-63588

There has been a memory inefficiency inside of do_blocks() where it parses the given content then iterates through each top-level block to render it. Unfortunately each top-level block can considerably add to the overall memory use involved, and once moving on to the next top-level block, all of the allocated memory will be retained until the end of the call to do_blocks().

In this change, each parsed block sub-tree is freed via reset to null after it has been rendered. All top-level blocks are rendered independently of each other and so this operation is safe — there are no data dependencies between them.

For a test post of length 2.3 MB containing 138 top-level blocks, this brought the minimum memory requirement for the PHP process down from around 450 MB to around 316 MB. This was “around” since the memory requirements are non-deterministic and some runs will succeed while other runs will crash for out-of-memory given the same memory_limit.

Impact of this change will be most profound when there exist one or more top-level blocks which allocate a significant amount of memory which are not the last top-level-block in a post. This means that this change might even impact small and typical posts, if the right blocks are near the start of the post.

[joemcgill]

This was committed in [60316]. Leaving open and marking as fixed-major and moving to the 6.8.2 milestone for backport consideration (cc @audrasjb).

I've reviewed the commit as well, so marked this as dev-reviewed.

[audrasjb]

This is definitely something we may want to fix in 6.8.2 👍

However, the fixed-major workflow keyword should only be added once the patch has been merged into trunk and is waiting for a branch backport, thus I'm removing it for now.

[audrasjb]

How strange, the related commit [60316] doesn't appear in the ticket… maybe because no milestone was assigned to it?

Re-adding fixed-major.

@dmsnell should we leave a comment there about the optimization? Seems like an easy thing that can be lost to the sands of time otherwise.

[joemcgill]

Replying to audrasjb:

How strange, the related commit [60316] doesn't appear in the ticket… maybe because no milestone was assigned to it?

There was some kind of delay with Trac, but it seemed to catch up after I made this update. See ​this related Slack convo.

[zunaid321]

This ticket was brought up in a recent bug scrub.

Here’s the feedback received:

1.⁠ ⁠Being such a fast report has not given time for testing. So its difficult to make an informed decision about this.

Thanks!

Props to: @sirlouen & @westonruter

[dmsnell]

Thanks all for the cleanup and tagging. I’m not sure what happened here either, as I saw that the commits weren’t being linked properly even though it was in the subversion repo. Then I was waiting to see if some process would catch up but someone else beat me to it.

[dmsnell]

I know the the first patch, review, and merge was fast, but I have just now also proposed #63515 to add a comment explaining the change. My reason for doing this today is to try and ship it coincident with the change itself into 6.9, and the beta window opens tomorrow. Please pardon the second patch, because I would like to see it merge quickly as well (although I did not ask for or expect the first patch to be reviewed and approved so quickly).

[dmsnell]

#63616 was marked as a duplicate.

Trac ticket: Core-63588
See #8999

Adds explanatory comment indicating why the optimization was added and guarding against accidental removal.

This is a documentation-only change and should include no functional or visual changes.

@audrasjb I’m open to that, though I would need help with knowing what link you’re referring to. do you mean removing this inline link entirely and adding another PHPDoc token below on a line of its own, * @see parse_blocks()?

This docblock won't be fetched by the docblock, parser as it is not a function or a hook

Right, so I think this only affects an in-IDE experience, for which I was trying to optimize.

providing the link is probably better than using a relative reference

I don’t think I know what you are referring to with “relative reference”. I did some testing an indeed, were this docblock inside a class it would resolve to the class method of the same name. With namespaces it follows the module namespace.

I’ll wait for clarification, but in the meantime I’ve added a leading \ which should resolve all ambiguity in referencing the name.

[dmsnell]

In 60400:

do_blocks(): Document transient-memory-leak optimization.

Adds explanatory comment indicating why the optimization was added
and guarding against accidental removal.

This is a documentation-only change and should include no functional
or visual changes.

Props audrasjb, dmsnell, joemcgill, sirlouen, westonruter, zunaid321.
Follow-up to [60316].
Fixes #63588.

[audrasjb]

Reopening for 6.8 backport.

And this is also a second committer sign-off for a branch backport, for both [60316] and [60400].

[jorbin]

In 60402:

Coding Standards: Fix alignment in do_blocks.

Introduced in [60316].

See #63588.

[zunaid321]

This ticket was brought up in a recent bug scrub.

Here's the feedback:

We are currently waiting for a second committer to push the ticket forward.

[dmsnell]

@audrasjb I may have misunderstood your sign-off. Should I go ahead and merge this into 6.8.2 then?

[audrasjb]

Yes absolutely, the ticket is ready to be backported.
We'll need to backport the 3 above changesets: [60316], [60400] and [60402].
I can handle the backport if you want :)

[dmsnell]

If you want to handle it please go ahead. I will otherwise attempt to do so tomorrow, so if it’s not done by then I’ll reach out in Slack to make sure we don’t do the same work, and then prepare a commit to merge the three changes. Thank you!

[audrasjb]

@dmsnell alright! Maybe you're already aware of that, but just wanted to let you know don't have to prepare manually the commit, you can merge all 3 commits at once using a command like svn merge -c XXXXX,YYYYY,ZZZZZ '^/trunk'.
​https://make.wordpress.org/core/handbook/best-practices/backporting-commits/

However, please rather use the Core Component as the first element of your commit message.
Instead of do_blocks(): Document transient-memory-leak optimization. the commit main description here should be Editor: Document transient-memory-leak optimization in do_blocks().

It makes commit messages easier to classify when we use their component name.
​https://make.wordpress.org/core/handbook/best-practices/commit-messages/#guidelines

[audrasjb]

In 60435:

Editor: Free up transient memory leak in do_blocks().

There has been a memory inefficiency inside of do_blocks() where it
parses the given content then iterates through each top-level block to
render it. Unfortunately each top-level block can considerably add to
the overall memory use involved, and once moving on to the next
top-level block, all of the allocated memory will be retained until the
end of the call to do_blocks().

In this change, each parsed block sub-tree is freed via reset to null
after it has been rendered. All top-level blocks are rendered
independently of each other and so this operation is safe — there are no
data dependencies between them.

This commit also includes follow-up changes committed in [60400] and [60402].

Rewieved by audrasjb.
Merges [60316], [60400] and [60402] to the 6.8 branch.
Props dmsnell, joemcgill.
Fixes #63588.