Avoid calling plugin_dir_url a lot

[colinleroy]

Hello,
I've spent a bit of time optimizing my Wordpress + Woocommerce install. I've noticed that Automattic\WooCommerce\Blocks\Domain\Package->get_url() is called quite a few times (79 times on my installation), and recomputes plugin_dir_url each time.
This patch caches it in a static variable. It is a little improvement, but makes this function responsible for about 71 microseconds instead of 3.5 milliseconds, so I suppose it's something :)

Hope this helps.

Callstack before:

Callstack after:

Testing instructions

Visit some pages containing blocks and ensure they load correctly. Check you can still add blocks in the block editor.

Changelog

Cache plugin_dir_url instead of recomputing it dozens of times.

[opr]

Hey there @colinleroy thanks for making this PR - it seems like a good improvement for sure. What do you think about moving this logic to the constructor and keep the $plugin_dir_url a field variable? This would be so we don't need to use the static keyword in the get_url method and we can keep get_url super simple.

Also, what are your thoughts on someone using the plugins_url filter somewhere in the middle of a request? How do you value the time improvement made here vs. potentially breaking sites that do this, however rare it might be?

[colinleroy]

Hey there @colinleroy thanks for making this PR - it seems like a good improvement for sure. What do you think about moving this logic to the constructor and keep the $plugin_dir_url a field variable? This would be so we don't need to use the static keyword in the get_url method and we can keep get_url super simple.

Hi @opr, thanks for your interest ! and thank you for the suggestion, this is cleaner your way! You can see I'm not quite acquainted with WP/WC's codebases yet :)

Also, what are your thoughts on someone using the plugins_url filter somewhere in the middle of a request? How do you value the time improvement made here vs. potentially breaking sites that do this, however rare it might be?

That's a very good question, I'm unsure of the answer ! First of all I think the time improvement is far less important than not breaking sites.
From my understanding, this code path is only taken when the various Woocommerce blocks initialize :

From the code, I think that all Woocommerce blocks initialize in sequence, from Woocommerce's main initialization. So I suppose no other plugin code is ran inbetween, which means either a plugins_url filter could run before, or after, but not in the middle of the blocks init iteration. So I think it is not a potential issue... at least I think that it doesn't change the current behaviour. But I'm not a hundred percent sure, it would be nice to have confirmation from developers who know the codebase.

[opr]

From the code, I think that all Woocommerce blocks initialize in sequence, from Woocommerce's main initialization. So I suppose no other plugin code is ran inbetween, which means either a plugins_url filter could run before, or after, but not in the middle of the blocks init iteration. So I think it is not a potential issue... at least I think that it doesn't change the current behaviour. But I'm not a hundred percent sure, it would be nice to have confirmation from developers who know the codebase.

Thank you for looking into it and writing up your thoughts.

I've run some tests to see if get_url is called when handling code that extends WooCommerce blocks, it doesn't look like it is. I'm quite satisfied that this change won't affect extensions because like you say, all of this happens at once and nothing else should get chance to add a filter for plugins_url.

Thanks for doing this, and making the amends. Pinging @mikejolley for another review and to confidence check our findings about whether there is going to be issues with the plugin url changing.

[mikejolley]

Looks good, thanks for making those changes.

[opr]

@colinleroy thanks for your work here, this improvement will be nice 😃 👍🏼

I added some testing instructions in your PR body, this is necessary for our release process.

[opr]

CI fails are due to it being a fork. Merging anyway and will make sure they pass on trunk.

[senadir]

FWIW this PR is causing a regression with symlinked WooCommerce installations.

[colinleroy]

Oh, I'm sorry about that! @shendy-a8c's fix seems good.