Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-10141

Performance improvement for $auth->_fill_acl()

    Details

    • Type: Improvement
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.8
    • Fix Version/s: 3.0.9-RC1
    • Component/s: Authentication
    • Labels:
      None
    • Environment:
      All

      Description

      When profiling phpBB $auth->_fill_acl() ended up near the top of the list. I've been looking through this function and it does a LOT of work and a large part of that work is redundant.

      _fill_acl() mainly translates the data in user_permissions in the users table to a bitstring with:

      $this->acl[$f] .= str_pad(base_convert($subseq, 36, 2), 31, 0, STR_PAD_LEFT);

      It does this a non trivial amount of times and $subseq is the same on most iterations. My change caches the results for the different subsequences:

      /**
         * Fill ACL array with relevant bitstrings from user_permissions column
         * @access private
         */
         function _fill_acl($user_permissions)
         {  
            $seq_cache = array();
            $this->acl = array();
            $user_permissions = explode("\n", $user_permissions);
       
            foreach ($user_permissions as $f => $seq)
            {  
               if ($seq)
               {  
                  $i = 0;
       
                  if (!isset($this->acl[$f]))
                  {
                     $this->acl[$f] = '';
                  }
       
                  while ($subseq = substr($seq, $i, 6))
                  {
                     if (isset($seq_cache[$subseq]))
                     {
                        $this->acl[$f] .= $seq_cache[$subseq];
                     }
                     else
                     {
                        // We put the original bitstring into the acl array
                        $this->acl[$f] .= ($seq_cache[$subseq] = str_pad(base_convert($subseq, 36, 2), 31, 0, STR_PAD_LEFT));
                     }
                     $i += 6;
                  }
               }
            }
         }
      

      The original _fill_acl() is here:

      https://github.com/phpbb/phpbb3/blob/master/phpBB/includes/auth.php#L129

      In my particular case this resulted in a 200% improvement for this function and a 10% saving in page generation times.

        Activity

        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        This looks reasonable. Do you have a preferred name and email address to use for this commit?

        Show
        Oleg Oleg [X] (Inactive) added a comment - This looks reasonable. Do you have a preferred name and email address to use for this commit?
        Hide
        BartVB BartVB [X] (Inactive) added a comment -

        Bart van Bragt <phpbb@vanbragt.com>
        Thanks.

        Show
        BartVB BartVB [X] (Inactive) added a comment - Bart van Bragt <phpbb@vanbragt.com> Thanks.
        Hide
        bantu Andreas Fischer added a comment -

        Unit tests would be nice, though.

        Show
        bantu Andreas Fischer added a comment - Unit tests would be nice, though.
        Hide
        bantu Andreas Fischer added a comment -

        Plus, I'm not sure whether I like how that assignment is written.

        Show
        bantu Andreas Fischer added a comment - Plus, I'm not sure whether I like how that assignment is written.
        Hide
        BartVB BartVB [X] (Inactive) added a comment -

        My code was written purely for speed. It's trivial to split the assignment in two, my guess is that the performance implications of that change are very limited but haven't benchmarked that.

        I'm not big on unit tests (in this context) so don't wait for me writing one

        Show
        BartVB BartVB [X] (Inactive) added a comment - My code was written purely for speed. It's trivial to split the assignment in two, my guess is that the performance implications of that change are very limited but haven't benchmarked that. I'm not big on unit tests (in this context) so don't wait for me writing one
        Hide
        bantu Andreas Fischer added a comment -

        Hey Bart. No we're certainly not expecting this (unit tests) from you.
        Here is another version, feel free to try it out. https://github.com/bantu/phpbb3/compare/ticket/10141

        Show
        bantu Andreas Fischer added a comment - Hey Bart. No we're certainly not expecting this (unit tests) from you. Here is another version, feel free to try it out. https://github.com/bantu/phpbb3/compare/ticket/10141
        Hide
        BartVB BartVB [X] (Inactive) added a comment -

        Nice I should have thought of that myself

        Show
        BartVB BartVB [X] (Inactive) added a comment - Nice I should have thought of that myself
        Hide
        Oleg Oleg [X] (Inactive) added a comment -

        If you really care about performance then https://github.com/p/phpbb3/commit/f49656986cc1898e85d6d7e4cd859ec8e980dc4a saves an unnecessary hash lookup when the value is not in cache.

        Inane issuance of warnings by php and the associated isset() calls probably make a good 25% of all hash lookups useless/redundant.

        Show
        Oleg Oleg [X] (Inactive) added a comment - If you really care about performance then https://github.com/p/phpbb3/commit/f49656986cc1898e85d6d7e4cd859ec8e980dc4a saves an unnecessary hash lookup when the value is not in cache. Inane issuance of warnings by php and the associated isset() calls probably make a good 25% of all hash lookups useless/redundant.

          People

          • Assignee:
            bantu Andreas Fischer
            Reporter:
            BartVB BartVB [X] (Inactive)
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development