NEXT-34070 - Improve seo url replacer

[akf-bw]

1. Why is this change necessary?

• In a production environment, the seo_url table can grow very quickly
• If the seo_url table contains a large amount of data combined with extensive navigation, it may take a while for the database query to find the required data
• Therefore it would be great if we could improve the database call

Before:

After:

(seo_url entries: ~380.000)

2. What does this change do, exactly?

• Changed the createSeoMapping method in SeoUrlPlaceholderHandler to use subqueries

3. Describe each step to reproduce the issue or behaviour.

• Fill the seo_url table with a lot of seo url's
• Open the storefront

4. Please link to the relevant issues (if any).

/

5. Checklist

• I have rebased my changes to remove merge conflicts
• I have written tests and verified that they fail without my change
• I have created a changelog file with all necessary information about my changes
• I have written or adjusted the documentation according to my changes
• This change has comments for package types, values, functions, and non-obvious lines of code
• I have read the contribution requirements and fulfil them.

[vienthuong]

Hi @akf-bw
Thanks for your contribution. The solution looks very good so far. I have a general question:
What could the overhead be in the original query? the path_info is already indexed. My guess is the problem might be there's no limit set for the original query? Could you try to set the limit for the two queries:

1. LIMIT = count($mapping) when sales_channel_id = :contextSalesChannelId
2. LIMIT = count($missing) when sales_channel IS null and $missing is not empty (as in your approach)

[akf-bw]

Hi @akf-bw Thanks for your contribution. The solution looks very good so far. I have a general question: What could the overhead be in the original query? the path_info is already indexed. My guess is the problem might be there's no limit set for the original query?

There was no limit in the original query. However, it was also not possible to add a limit in the original query because that query retrieved filled salesChannelId's and empty salesChannelIds at the same time, so there are multiple lines for a path_info.
Since we only need a single line for “path_info” in the next foreach, it is much better to use the two queries instead.

In my local test environment, the original query returned a list of approximately 110,000 rows per request.
The new two queries only return rows for the maximum size of the $missing array.

Could you try to set the limit for the two queries:

1. LIMIT = count($mapping) when sales_channel_id = :contextSalesChannelId
2. LIMIT = count($missing) when sales_channel IS null and $missing is not empty (as in your approach)

I don't need to add an additional limit to the new queries because the $mapping variable already defines how many rows are possible.

[vienthuong]

110,000 rows per request

Yeah I mean to set the limit in the original query and call it twice with different salesChannelId parameters (similar to your solution but we don't need the subqueries). But your sub-query solution would also be good.

Could you add the unit test to cover your changes?

[akf-bw]

Yeah I mean to set the limit in the original query and call it twice with different salesChannelId parameters (similar to your solution but we don't need the subqueries). But your sub-query solution would also be good

I already tested this. For this to work you also need to add "GROUP BY path_info" so that the limit can work correctly. This grouping is extremely slow and took about twice as long as the old query, so this wasn't a solution.

Could you add the unit test to cover your changes?

I'll take a look at this.

[akf-bw]

@vienthuong There is already the testSeoReplacementSalesChannelDefaultAndOverride method in SeoUrlPlaceholderHandlerTest which should cover the full behavior of the changed method.
In addition, there are already many other test classes that work with the SeoUrlPlaceholderHandler and all pass in the unit tests.

[akf-bw]

@vienthuong I again tested with the query params. If we include the params directly like this, we could save up to 1-5ms per createSeoMapping call:

Should I change the method to this?

[vienthuong]

@vienthuong I again tested with the query params. If we include the params directly like this, we could save up to 1-5ms per createSeoMapping call: Should I change the method to this?

In this particular case, the performance difference is not significant and using Uuid::fromHexToBytes would be safer. We will also evaluate it if it would be a good idea and worth the effort to use concatted string instead of the hex2bin method as we have used it everywhere

[akf-bw]

@vienthuong I just removed one of the foreach loops since we can move the behavior into the loop beforehand.

[vienthuong]

Hi @akf-bw, looks very good so far.
One last thing since I'm curious about your concatted-string solution vs Uuid::fromHexToBytes, could you do a little benchmark with your particular case with your 380k (or more) seo_urls?

[ghost]

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-34070

Please use this issue to track the state of your pull request.

[vienthuong]

We did evaluate the performance gains by using/not using Uuid::fromHexToBytes and it's insignificant unless you have millions of UUIDs. So for now we will keep using the method to have safer/standard UUIDs in the query.

[tamvt]

Your PR has been merged. Thank you for your contribution 🎉 💙

[En0Ma1259]

I've tried this change and now the query is slower ...

Mappings:
20: ~2ms ⇾ ~6ms
~100: ~10ms ⇾ ~45ms
~600: ~40ms ⇾ ~260ms

Shopware: 6.5.8.6
PHP: 8.2
MySQL: 8.0.35
Profiler: Blackfire
seo_url entry count: ~200,000

[vienthuong]

Hi @akf-bw could you also benchmark your instance with the latest source code?

[akf-bw]

@tamvt @vienthuong Could you please revert the PR changes in the trunk branch for now. I'll have to check what caused this different behavior in my environment.
As soon as I know more and have found a better/different optimization I will open a new PR.