array_merge_recursive inefficient on PHP 8.2 in HtmlForm
Open, Needs TriagePublicBUG REPORT
Actions

Assigned To

None

Authored By

	Universal_Omega
	Aug 14 2023, 8:04 PM

Description

What happens?:
HtmlForm (includes/htmlform/HtmlForm.php#L432) can lead to PHP memory exhaustion (apparently no matter how high I keep increasing it). This is due to the line:
$this->mFieldTree = array_merge_recursive( $this->mFieldTree, $loadedDescriptor ); array_merge_recursive seems to be highly inefficient in this instance, and it ends up working properly without memory exhaustion or even coming close if I change it to a function such as:

private function mergeArraysRecursive($array1, $array2) {
    foreach ($array2 as $key => $value) {
        if (is_array($value) && isset($array1[$key]) && is_array($array1[$key])) {
            $array1[$key] = $this->mergeArraysRecursive($array1[$key], $value);
        } else {
            $array1[$key] = $value;
        }
    }

    return $array1;
}

and replace array_merge_recursive with $this->mergeArraysRecursive

Software version: MW 1.40/PHP 8.2.8

Event Timeline

Universal_Omega created this task.Aug 14 2023, 8:04 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 14 2023, 8:04 PM

Paladox subscribed.Aug 14 2023, 8:08 PM

Izno added a project: Performance Issue.Aug 14 2023, 8:41 PM

Might be worth an upstream report if a minimal reproducer can be isolated.

Are you using the JIT, just in case?

In T344189#9092851, @TK-999 wrote:

Might be worth an upstream report if a minimal reproducer can be isolated.

Are you using the JIT, just in case?

Yes, JIT is being used.

I've heard array_merge_recursive is not great performance in PHP for quite a while for large arrays anyway, I wonder if something similar to StringUtils::explode() be added to ArrayUtils would be worth it for this?

Possibly. I think it's worth disabling the JIT (ie making sure that opcache.jit_buffer_size is unset or 0, which is the default) though just to rule it out as a cause—it's been known to cause oddball issues when it is enabled that simply do not occur when it is off, and its maintenance status in general is dubious.

In T344189#9093734, @TK-999 wrote:

Possibly. I think it's worth disabling the JIT (ie making sure that opcache.jit_buffer_size is unset or 0, which is the default) though just to rule it out as a cause—it's been known to cause oddball issues when it is enabled that simply do not occur when it is off, and its maintenance status in general is dubious.

Yeah, I'll give that a try, but I also know array_merge_recursive is one of the least performent functions in PHP even prior to PHP 8 as a whole, so I wonder if that is something that would be worth it even if the underlying exhaustion does not happen without JIT.

In T344189#9093734, @TK-999 wrote:

Possibly. I think it's worth disabling the JIT (ie making sure that opcache.jit_buffer_size is unset or 0, which is the default) though just to rule it out as a cause—it's been known to cause oddball issues when it is enabled that simply do not occur when it is off, and its maintenance status in general is dubious.

That unset for us doesn’t fix it.

R4356th subscribed.Oct 25 2023, 6:09 AM

What page(s) does this happen on reliably?

In T344189#9279123, @TK-999 wrote:

What page(s) does this happen on reliably?

I can reproduce this when saving on Special:ManageWiki/permissions but I think it happens to most of ManageWiki because it uses HtmlForm. The cause of this is https://github.com/wikimedia/mediawiki/commit/4727ed1a9ccb6c321e536f0249dafc1f4d160d88.

Can confirm the above also happens using 8.1, when also using the same extension.

Digging, I can reproduce the OOM if I just do: error_log(print_r($loadedDescriptor, true));.

HTMLForm has been widely used in the core and many extensions, if the bug is only reproducible with interfaces created by the ManageWiki extension, there must be some unusual pattern in that extension.

In T344189#9279380, @Func wrote:

HTMLForm has been widely used in the core and many extensions, if the bug is only reproducible with interfaces created by the ManageWiki extension, there must be some unusual pattern in that extension.

First It only happens on php 8.2/8.1. Secondly I can reproduce by just doing error_log(print_r($loadedDescriptor, true)); within addFields(). So it seems $loadedDescriptor is the cause.Thirdly it only happened when saving.

I don't see anything in https://github.com/miraheze/ManageWiki/blob/3ac3e7b69b911d97d22057e4b1541bc9d59d8374/includes/FormFactory/ManageWikiFormFactory.php#L54 that would cause exceptional memory usage?

Paladox added a comment.Oct 25 2023, 12:14 PM

This comment was removed by Paladox.

Even further, I got to $setSection[$fieldname] = $field;.

... And further I get to $descriptor['parent'] = $parent; (which the param passed is $this.

I can reproduce it on Special:Preferences.

Found it. It's the type in loadInputFromParameters.

Switch from:

	public static function loadInputFromParameters( $fieldname, $descriptor,
		HTMLForm $parent = null
	)

	public static function loadInputFromParameters( $fieldname, $descriptor, $parent = null)

seems to fix it.

In T344189#9283792, @Paladox wrote:
Found it. It's the type in loadInputFromParameters.

Switch from:
	public static function loadInputFromParameters( $fieldname, $descriptor,
		HTMLForm $parent = null
	)
to
	public static function loadInputFromParameters( $fieldname, $descriptor, $parent = null)
seems to fix it.

Hmm, doesn't work now. (I've reset the file back and tried applying it to make sure)

We could revert https://github.com/wikimedia/mediawiki/commit/4727ed1a9ccb6c321e536f0249dafc1f4d160d88 until a fix is found?

I found a fix: https://github.com/miraheze/ManageWiki/commit/4fb5fd1a365b55889b632d11ed07239f2582c1bf. Not sure what changed in php 8.2 to require that. I saw on production:

which led me to finding this fix (prod was running php 7.4).

	F41446891: image.png
	Nov 4 2023, 3:27 PM

array_merge_recursive inefficient on PHP 8.2 in HtmlFormOpen, Needs TriagePublicBUG REPORTActions

Description

Event Timeline

array_merge_recursive inefficient on PHP 8.2 in HtmlForm
Open, Needs TriagePublicBUG REPORT
Actions