Don't use array merge here.

[okonomiyaki3000]

Pull Request for Issue # .

Summary of Changes

Instead of:
create array
merge new array into old array to create another array
replace old array with newly merged array

Just:
push item onto old array

Testing Instructions

1. Check that your php max_execution_time is something sensible like 30 seconds.
2. Turn on finder indexing for content.
3. Create a super long article. Maybe like 10,000 words. Maybe this can help: http://slipsum.com/
4. Save the article.

Expected result

Well, I expect the article to save properly and the page to reload without crashing.

Actual result

Most likely, the article will save but in the post-save indexing process there will be a timeout. This happens because we are looping over a huge array of tokens and adding each one to the values array of the query object. The query object handles this rather inefficiently by creating an array, merging it with an existing array to create a third array and then replacing the existing array with the new one when all it needed to do was a push. This gets very bad when the existing values array is already very large.

There is another half of this function which is when an array is passed to it instead of a string. This would probably be a lot better like:

foreach ($elements as $element)
{
    $this->elements[] = $element;		
}

Anybody agree?

Documentation Changes Required

nope

[ggppdk]

It is great, if you know that you do not have duplicates
so your proposed code is good only if you know you will never have duplicates

[EDIT]
i removed code description after this point it was irrelevant

[okonomiyaki3000]

@ggppdk None of what you have said seems to apply to this situation at all.

[ggppdk]

@okonomiyaki3000

None of what you have said seems to apply to this situation at all.

i only said 1 thing , i spoke of duplicates
do you mean there is no chance that this method will need to handle duplicates ?

[ggppdk]

on what i agree with you, is that
-- this performance issue really needs to be addressed

[okonomiyaki3000]

@ggppdk array_merge has nothing to do with duplicates unless you mean duplicate keys when using associative arrays. This function has nothing to do with associative arrays.

[csthomas]

For me function $array = array_merge($array, array($oneValue)) do the same as $array[] = $oneValue;

Off topic: $array = $array + array($oneValue); is not equal to the above.

[ggppdk]

@okonomiyaki3000

array_merge has nothing to do with duplicates unless you mean duplicate keys when using associative arrays.

yes, you were right and i was wrong, i remembered wrongly its behaviour ... i was thinking of its behaviour when keys are strings, in which case it replaces the values of the first array if same index exists in the second
... aka no duplicate keys (and i was wrongly thinking of no duplicate values)

[okonomiyaki3000]

@csthomas for you, maybe the same. For PHP, not the same.

Let's break down $array = array_merge($array, array($oneValue));
First, before anything happens, you've got an array $array and it might be real big.
Then, the first thing that actually happens, you make a new array array($oneValue) (for no good reason)
Next, you create another array by array_merge($array, array($oneValue)). At this point, you still have $array (which might be big) but now you also have a slightly bigger array. You have also called a function which carries a certain amount of overhead in and of itself.
Finally, you will replace $array with the slightly bigger array you have created by merging.

This all happens very fast but try doing it 10,000 times with ever increasing array sizes.

When you do $array[] = $oneValue;, you don't create any new arrays and you don't call any functions. The end result may be the same but it's obviously much faster which is the whole point.

[ggppdk]

Indeed in this case we have numerical indexes and not string indexes (?)
so @okonomiyaki3000 suggested change should be good

[csthomas]

@okonomiyaki3000 Yes I understand it and whole PR very well.
I only wanted to say that end result will be the same, so there is no problem.

[ggppdk]

99% of the time i avoid using array_merge because of its unneed bad performance, especially when code will be called repeatedly, and that made me post wrong comment above

This PR looks like a very good performance improvement

[okonomiyaki3000]

The only question for me is, should we also change the rest of this function to be like:

foreach ($elements as $element)
{
    $this->elements[] = $element;		
}

I think we probably should, I just hope I'm not missing anything.

In fact, it might be better to just rewrite the whole function as:

public function append($elements)
{
    foreach ((array) $elements as $element)
    {
        $this->elements[] = $element;
    }
}

Any opinions about it?

[ggppdk]

... Any opinions about it?

Looking at it more carefully

The change you have done so far seems 100% safe
because you have changed the case that a single string is appended

If you also want to change the case that an array of ($elements) will be appended
(making a single loop to handle both cases)
then it will be assumed that the method is given always arrays with numerical keys

Currently the class is written with ability to preserve non-numerical keys,
but it seems that this is not needed
since all code is passing strings, or arrays with numerical keys (not 100% sure of this)
(except for sqlite codewhich in some cases it will pass column names)

[csthomas]

I have tested this item ✅ successfully on eb7264a

I tested on one long article (99 paragraphs) with plugin smart search - content enabled.
Before patch article saved for ~10 second. After patch saved for ~3 seconds.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17391.}

[ggppdk]

I have tested this item ✅ successfully on eb7264a

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17391.}

[ghost]

RTC after two successful tests.