Uploaded image for project: 'CiviCRM'
  1. CiviCRM
  2. CRM-13288

CRM_Utils_Array::value(): Not outsmarting PHP + Optimization

    Details

    • Type: Improvement
    • Status: Done/Fixed
    • Priority: Trivial
    • Resolution: Incomplete
    • Affects Version/s: 4.3.5
    • Fix Version/s: Unscheduled
    • Component/s: Core CiviCRM
    • Labels:
    • Documentation Required?:
      None
    • Funding Source:
      Needs Funding

      Description

      CRM_Utils_Array::value is the most used function in CiviCRM, being called roughly 1,000 times on every single page load. So it is key to make it right, and fast.

      Here is the code:
      static function value($key, $list, $default = NULL) {
      if (is_array($list))

      { return array_key_exists($key, $list) ? $list[$key] : $default; }

      return $default;
      }

      I have an issue with the way the second parameter is declared. It should really be '&$list' rather than simply '$list' since we really do not intend to make a copy of the array. I suppose this is intentional in order to optimize speed of execution due to PHP's copy-on-write mechanism that will not actually make a copy of the parameter until it is actually written into.

      However there is a dangerous assumption that this behavior will not change in further versions of PHP. As a reminder it did change several times in the past. I think by far the better way would be to not try to outsmart PHP by relying on undocumented features and actually implement what is syntaxly and philosophically correct, namely '&$list'.

      As a side note most if not all of the other functions in CRM_Utils_Array do have the reference '&$list'.

      Finally, in order to improve execution times, it is well documented that isset() is at least 2.5x faster than array_key_exists, so we could do:
      return (isset($list[$key]) || array_key_exists($key, $list)) ? $list[$key] : $default;

      See http://www.zomeoff.com/php-fast-way-to-determine-a-key-elements-existance-in-an-array/ for reference and concrete benchmarks with the above.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                colemanw Coleman Watts
                Reporter:
                nganivet Nicolas Ganivet
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Time Tracking

                  Estimated:
                  Original Estimate - 30 minutes
                  30m
                  Remaining:
                  Remaining Estimate - 30 minutes
                  30m
                  Logged:
                  Time Spent - Not Specified
                  Not Specified