Speed up loading menu params

[gjedeer]

I'm working on improving performance of a Joomla site. I noticed that the longest part of generating a page is JApplicationCms->getMenu(). The query returns 380 menu items, each of them with a larger than usually params column (some extension). JApplicationCms took 1.78s to execute, out of which 1.37s has been spent in 489 calls of Registry::bindData.

Essentially, for menu items, the flow is as following:

JMenu loads data rows, params is a JSON string
JRegistry loadString() converts this string to an object
JRegistry bindData() converts that object to an array
JRegistry bindData() creates a new object, iterates over the array and copies keys as object properties
The newly created object is returned

I propose that for simple cases like loading a menu item, the flow looks like this:

JMenu loads data rows, params is a JSON string
JRegistry loadString() converts this string to an object
The object is returned

After my changes, said site takes 0.21s to execute JApplicationCms->getMenu().

[mbabker]

At a glance something doesn't seem right with this pull request (aside from the CI failures). Part of what the bindData() method does is ensure that the internal data storage is structured correctly. Your changes bypass this entirely and just assigns the raw injected data as the internal data store. Does this in any way corrupt the data store for the resulting Registry object?

[gjedeer]

In case of loading JSON from string, bindData() is called with recursive=TRUE, allowNull=TRUE so it's effectively copying the input object.

The only validation I see in bindData() is checking for empty values, which is disabled in that code path (allowNull=TRUE) and ensuring that nested objects are of type stdClass, which is always true when loading from string.

[gjedeer]

Does this in any way corrupt the data store for the resulting Registry object?

The site works normally. I'm not sure I understand the question.

The shortcut I'm proposing is not executed when Registry is working on an already existing object - only when an empty object has been created in constructor.

[mbabker]

I'd need to get a data set together and test it to verify everything's fine. I'm not opposed to this PR if it improves performance as you say it does for larger registries, I just want to make sure the data structure doesn't get corrupted along the way.

[mbabker]

Sorry it took so long to get back to this but after doing some tests all seems well. So made a couple tweaks to the empty data property and it's good to go.

[gjedeer]

Thanks! When do you think this will land in Joomla, in some 3.5 minor release or in the next major one?

[mbabker]

Probably in 3.6 since they aren't planning anymore 3.5 releases.

[wilsonge]

So this breaks the majority of the frontend of the CMS it would appear joomla/joomla-cms#11334

[gjedeer]

Interesting, the patch is running on one installation since the time i opened this PR, no issues at all.

On July 28, 2016 9:34:52 PM CEST, George Wilson notifications@github.com wrote:

So this breaks the majority of the frontend of the CMS it would appear
joomla/joomla-cms#11334

---

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#36 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[wilsonge]

Don't stress - turns out the issue wasn't due to this PR after all :) Should have posted a follow up here. Mea culpa!

[gjedeer]

whew good to hear!