While each is deprecated, Foreach replacement takes a lot more time

Question

With PHP 7.2, each is deprecated. The documentation says:

Warning This function has been DEPRECATED as of PHP 7.2.0. Relying on this function is highly discouraged.

I am working on adapting an e-commerce application and converting all while-each loops to the (supposedly) equivalent foreach.

As you can see below, I have already replaced all the reset & while loops with an equivalent foreach.

It mostly worked fine. However, we had a customer with a very long list of items in her cart that tried to checkout and complained that she gets error 502 from the server. I tried to reproduce that and found that only her cart fails, it takes over 2 minutes for the checkout page to load, and then times with a 502. I then started debugging a lot of files I recently modified, trial and error, until I found out that the issue is with this specific file and specific function. Whenever I switch the first foreach loop back to the while loop, the customer can load the checkout page in less than a second. Switching back to foreach - and again it take minutes, but php times out before it ends execution.

I did perform tests of course on the output of that foreach vs the while loop (var_dump $products_id and $this->contents for example) and they all seem identical. I already rewrote the code to make it work smoothly and to stay PHP 7.2 compatible, but I still can't figure out why this happens.

This is the full function:

function get_content_type() {
  $this->content_type = false;

  if ( (DOWNLOAD_ENABLED == 'true') && ($this->count_contents() > 0) ) {

    // reset($this->contents);
    // while (list($products_id, ) = each($this->contents)) {
    foreach(array_keys($this->contents) as $products_id) {

      if (isset($this->contents[$products_id]['attributes'])) {
        // reset($this->contents[$products_id]['attributes']);
        // while (list(, $value) = each($this->contents[$products_id]['attributes'])) {
        foreach ($this->contents[$products_id]['attributes'] as $value) {
          $virtual_check_query = tep_db_query("select count(*) as total from " . TABLE_PRODUCTS_ATTRIBUTES . " pa, " . TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD . " pad where pa.products_id = '" . (int)$products_id . "' and pa.options_values_id = '" . (int)$value . "' and pa.products_attributes_id = pad.products_attributes_id");
          $virtual_check = tep_db_fetch_array($virtual_check_query);

          if ($virtual_check['total'] > 0) {
            switch ($this->content_type) {
              case 'physical':
                $this->content_type = 'mixed';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'virtual';
                break;
            }
          } else {
            switch ($this->content_type) {
              case 'virtual':
                $this->content_type = 'mixed';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'physical';
                break;
            }
          }
        }

      } elseif ($this->show_weight() == 0) {
      // reset($this->contents);  
      //  while (list($products_id, ) = each($this->contents)) { 
        foreach (array_keys($this->contents) as $products_id) {
          $virtual_check_query = tep_db_query("select products_weight from " . TABLE_PRODUCTS . " where products_id = '" . $products_id . "'");
          $virtual_check = tep_db_fetch_array($virtual_check_query);
          if ($virtual_check['products_weight'] == 0) {
            switch ($this->content_type) {
              case 'physical':
                $this->content_type = 'mixed';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'virtual';
                break;
            }
          } else {
            switch ($this->content_type) {
              case 'virtual':
                $this->content_type = 'mixed';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'physical';
                break;
            }
          }
        }

      } else {
        switch ($this->content_type) {
          case 'virtual':
            $this->content_type = 'mixed';

            return $this->content_type;
            break;
          default:
            $this->content_type = 'physical';
            break;
        }
      }
    }
  } else {
    $this->content_type = 'physical';
  }

  return $this->content_type;
}

Thank you

EDIT: here is the array: https://pastebin.com/VawX3XpW

The issue has been tested and reproduced on all the configurations I have tried:

1) High end windows 10 pc + WAMP (Apache 2.4 + MariaDB 10.2 + PHP 5.6+/7+/7.1+/7.2+)

2) High end CentOS/cPanel server + Litespeed + MariaDB 10.1 + PHP 5.6+

Just to emphasize, I am not looking to rewrite the code or emulate each and then rewrite the code, as we won't learn much from that. I am simply trying to find a logical explanation or a way to solve/debug this mystery. Maybe someone, somewhere, ever bumped into such an issue and can shed some light on this.

UPDATE 01/Aug/2018

I have been trying to debug this for days, eventually noticed something interesting. I added "echo points" and exit on the first foreach loop and while loop like so:

function get_content_type() {
  $this->content_type = false;

  if ( (DOWNLOAD_ENABLED == 'true') && ($this->count_contents() > 0) ) {

    // reset($this->contents);
    // while (list($products_id, ) = each($this->contents)) { echo '1 ';
    foreach(array_keys($this->contents) as $products_id) { echo '1 ';

      if (isset($this->contents[$products_id]['attributes'])) { echo '2 ';
        // reset($this->contents[$products_id]['attributes']);
        // while (list(, $value) = each($this->contents[$products_id]['attributes'])) {
        foreach ($this->contents[$products_id]['attributes'] as $value) { echo '3 ';
          $virtual_check_query = tep_db_query("select count(*) as total from " . TABLE_PRODUCTS_ATTRIBUTES . " pa, " . TABLE_PRODUCTS_ATTRIBUTES_DOWNLOAD . " pad where pa.products_id = '" . (int)$products_id . "' and pa.options_values_id = '" . (int)$value . "' and pa.products_attributes_id = pad.products_attributes_id");
          $virtual_check = tep_db_fetch_array($virtual_check_query);

          if ($virtual_check['total'] > 0) {
            switch ($this->content_type) {
              case 'physical':
                $this->content_type = 'mixed'; echo '4 ';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'virtual'; echo '5 ';
                break;
            }
          } else {
            switch ($this->content_type) {
              case 'virtual':
                $this->content_type = 'mixed'; echo '6 ';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'physical'; echo '7 ';
                break;
            }
          }
        }

      } elseif ($this->show_weight() == 0) {
      // reset($this->contents);  
      //  while (list($products_id, ) = each($this->contents)) { 
        foreach (array_keys($this->contents) as $products_id) {
          $virtual_check_query = tep_db_query("select products_weight from " . TABLE_PRODUCTS . " where products_id = '" . $products_id . "'");
          $virtual_check = tep_db_fetch_array($virtual_check_query);
          if ($virtual_check['products_weight'] == 0) {
            switch ($this->content_type) {
              case 'physical':
                $this->content_type = 'mixed'; echo '8 ';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'virtual'; echo '9 ';
                break;
            }
          } else {
            switch ($this->content_type) {
              case 'virtual':
                $this->content_type = 'mixed'; echo '10 ';

                return $this->content_type;
                break;
              default:
                $this->content_type = 'physical'; echo '11 ';
                break;
            }
          }
        }

      } else {
        switch ($this->content_type) {
          case 'virtual':
            $this->content_type = 'mixed'; echo '12 ';

            return $this->content_type;
            break;
          default:
            $this->content_type = 'physical'; echo '13 ';
            break;
        }
      }
    } exit; //Exiting from the loop to check output
  } else {
    $this->content_type = 'physical';
  }

  return $this->content_type;
}

When I was running the loop with while, the output I got was just "1 13" once, meaning the loop is running only once and stops. However, when I changed it to foreach, I got a long list of "1 13 1 13 1 13...", meaning it is looping many times. I have gone to investigate further if there is any difference between breaks of while loops and foreach loops, and I still couldn't find any supporting information. I then rewrote the last break; to be break 2; and tested the foreach again and this time it seems to have been running just once, just like when it was a while loop with a break; (not break 2;) EDIT: Just to clarify - there is no difference between while breaks and foreach breaks. They work the same way.

UPDATE #2: I have revised } elseif ($this->show_weight() == 0) { to } elseif (2 == 0) { and the while loop now runs as many times as the foreach loop. var_dump($this->show_weight()); results float 4466.54. The issue still doesn't make any sense to me.

Thanks again

When the checkout page was taking 2 minutes to load, what was the visual result? Was it normal, or was there many repeated items or was it truncated maybe (indicating some kind of infinite loop)? — rlanvin, Aug 01 '18 at 17:57
@rlanvin first, it crashed after around 45s due to memory exhaustion. When I increased that value, it timed out due to 120s limit, then I increased this more and eventually it did load after a long while, details on the page seemed normal. — Nikita 웃, Aug 01 '18 at 18:00
`foreach` is actually a very slow way to loop over an array in most situations, as compared to `for` with a precalculated `count`. See http://www.phpbench.com. Rewriting your `foreach` loops as `for` loops would likely result in a significant improvement in performance, but you should test this, obviously. — elixenide, Aug 06 '18 at 18:15

rlanvin · Accepted Answer · 2018-08-07T08:22:07.957

15

This is actually a very simple algorithmic problem, and it has to do with the fact that when show_weight() is 0 you are looping the same array (edit: and based on your comments, show_weight() itself is also looping the same array).

TL;DR With while all those loops were sharing the same internal pointer and influencing each others. With foreach each loop is independent and therefore it runs way more iterations, hence the performance issue.

Since an example is worth a thousand words, hopefully the following code will make things clearer:

<?php

$array = ['foo','bar','baz'];

foreach ( array_keys($array) as $key ) {
    echo $array[$key],"\n";
    foreach ( array_keys($array) as $key ) {
        echo "\t",$array[$key],"\n";
    }
}

echo "---------------\n";

while ( list($key,) = each($array) ) {
    echo $array[$key],"\n";
    reset($array);
    while ( list($key,) = each($array) ) {
        echo "\t",$array[$key],"\n";
    }
}

This will output:

foo
        foo
        bar
        baz
bar
        foo
        bar
        baz
baz
        foo
        bar
        baz
---------------
foo
        foo
        bar
        baz

As you can see, for an array of size 3, foreach takes 3² iterations, while while takes only 3. That's your performance problem.

Why was `while` faster?

Because at the end of the second (inner) while, the internal pointer of $array would have been pointing to the end of the array, therefore the first (outer) while would simply stop.

With a foreach, since your are using the result of 2 different calls to array_keys, you are using 2 different arrays that do no share the same internal pointer, therefore there is no reason for the loop to stop. A simple return after the second (inner) foreach should solve the problem.

edited Aug 07 '18 at 08:22

answered Aug 01 '18 at 18:35

rlanvin

6,057
2
18
24

Thanks. Note that I did try to change all the parts with `array_keys` like `foreach(array_keys($this->contents) as $products_id) {` to `foreach($this->contents as $products_id => $val) {` with the same issue. Does that change your theory about the `while`? – Nikita 웃 Aug 01 '18 at 18:39
by the way, `show_weight()` is never 0. It's always `4466.54`, at least in this specific test case on this specific cart. It is actually pretty stupid that the original code author put that `show_weight()` heavy function inside the loop, as it was enough to assign it to a variable outside the loop and then check the variable's value inside the loop to save all the queries and computation. But the discussion is not about how to improve/optimize the code. I assume this information still isn't related to why there is such a difference between the `while` and the `foreach`. – Nikita 웃 Aug 01 '18 at 18:54
No it does not change my theory. `foreach` keeps a separate pointer (at least in PHP 7, not sure about PHP 5) while the two `while` are sharing the same pointer (try it yourself with my example code above). How did you test that `show_weight` is never 0 is this specific test case? Also I'm assuming it is the test case that demonstrate the problem, right? – rlanvin Aug 01 '18 at 19:49
@CM웃 Actually the content of `show_weight` itself could be relevant, and I'm willing to bet you will find `current($this->contents)` inside. – rlanvin Aug 01 '18 at 20:00
Thanks. I think the answers to your questions (and your bet) are in the actual full class, which you can see here: https://pastebin.com/6fBZiYY8 . Yes, the test case is of course the case that demonstrate the problem. Does that script still support your opinion? – Nikita 웃 Aug 02 '18 at 08:05
2

Yes, just read it! `show_weight` calls `calculate` which does **another nested loop on the same array**. This is *exactly* what I explained in my answer. With `while` all those loops were sharing the same internal pointer and influencing each others. With `foreach` each loop is independent and therefore it runs *way* more iterations, hence the performance issue. – rlanvin Aug 02 '18 at 15:40
2

Spot on and Excellent answer! This should serve as a huge notice for anybody converting `while` loops to `foreach` loops and thinking it is an identical drop-in replacement. Especially now that php 7.2 is out. – Nikita 웃 Aug 08 '18 at 14:43
2

@rlanvin is spot on in the analysis. It's sort of like having this nested loop: for ($i = 0; $i < 1000; ++$i) { for ($i = 0; $i < 1000; ++$i) { // do stuff } } And turning it into this: for ($i = 0; $i < 1000; ++$i) { for ($j = 0; $j < 1000; ++$j) { // do stuff } } Then wondering why the second version takes 1000x times as long. The code you had before was probably "efficiently" hiding some subtle correctness bug (or maybe not) by mutating the internal pointer from two different places. This is a largely WHY we deprecated each(), it's easy to get wrong – Sara Aug 26 '18 at 01:27
1

Additional note for above: I personally voted against the each() deprecation, because your code was working and we didn't need to break it. Sorry. -Sara (PHP 7.2 Release Manager) – Sara Aug 26 '18 at 01:33
Unrelated: why are you iterating over `array_keys` rather than the array itself? – shalvah Aug 26 '18 at 09:23

score 3 · Answer 2 · answered Jul 17 '18 at 21:27

3

As a backup solution, What about a drop in replacement performing the same action?

function eachLegacy( &$array )
{
  if( current( $array ) !== false )
  {
    $return = array( 1 => current( $array ), 'value' => current( $array ), 0 => key( $array ), 'key' => key( $array ) ); // Get the current values
    next( $array ); // Advance the cursor
    return $return;
  }
  return false;
}

answered Jul 17 '18 at 21:27

Scuzzy

12,186
1
46
46

2

Thanks, @Scuzzy, I am not looking for a way to emulate `each` or to revise the code as I already done that. Also I have already seen this `each` emulation answer here: https://stackoverflow.com/questions/46492621/how-to-resolve-this-deprecated-function-each-php I really want to know what's causing this mystery and/or how to debug it – Nikita 웃 Jul 17 '18 at 21:31
1

Sound approach, I'll post another answer if I can see anything, but I can't think of what would lead to differences between a while loop and a foreach loop like you've stated. good luck. – Scuzzy Jul 17 '18 at 21:31

Abdelhafid El Kadiri · Answer 3 · 2018-08-05T20:09:43.013

I don't quite understands the main problem here but, you can start by optimizing your foreach loops using key value approach:

foreach($this->contents as $products_id => $products_value) {
    echo '1 ';
    if (isset($products_value['attributes'])) {
        echo '2 ';
        foreach ($products_value['attributes'] as $value) {
            echo '3 ';
            ...

using return in switch will break the whole function, exit also. if you want to break a loop from a switch statement use:

break NUMBER_OF_PARENT_STATEMENTS;

break 2; will break switch and the parent foreach

break 3; will break switch and the first and the second parent foreach

Azkatro · Answer 4 · 2018-08-06T05:01:20.717

1

In your original code, if the following returns true in the first iteration:

$this->show_weight() == 0

The code loops through $this->contents using each(), setting the array pointer on $this->contents to the end. So when we get back around to the first while() statement, it assumes it's already done, because the next call of each($this->contents) returns false.

edited Aug 06 '18 at 05:01

answered Aug 06 '18 at 04:46

Azkatro

63
8

While each is deprecated, Foreach replacement takes a lot more time

4 Answers4

Why was while faster?

Why was `while` faster?