From bc70bd69edb651c51fa12eb24e25d6eaa8068315 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 6 Sep 2023 20:38:05 +0200 Subject: [PATCH 1/3] MDL-79285 xhprof: Enable reducing runs data for quicker use Here we are reducing the xhprof runs data by removing the __Mustache==>__Mustache calls and all the orphaned data. To save N iterations what we are doing is: 0. The information is "topologically" sorted, so we ensure that all the parents in the data are processed before the children. (this will help a lot when cleaning orphaned data, see below). 1. First pass, all the candidate (by regexp) calls are removed from the run data. 2. Second pass, all the orphaned information (calls that have ended losing his parent) are also removed, so data is consistent. Note that, normally we would need N passes to remove all the orphaned data (because each pass creates new orphan candidates), but, as far as we have ensured that the information is topologically sorted (see point 0 above), all this can be done in one unique pass. TODO: - Add unit tests. - Enable some system to be able to decide which utilities we want to get the data reduced and which ones will continue using the complete data. Right now the reduction is being applied to all the utilities (both table and graph views). - Document the change and, if implemented, the way to select between complete/reduced data. - Consider adding some caching to speed-up the reduction process (some TODOs have been left in the code pointing to the critical points). --- lib/xhprof/xhprof_moodle.php | 139 ++++++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 2 deletions(-) diff --git a/lib/xhprof/xhprof_moodle.php b/lib/xhprof/xhprof_moodle.php index b87916dbdd2..cd33d3be037 100644 --- a/lib/xhprof/xhprof_moodle.php +++ b/lib/xhprof/xhprof_moodle.php @@ -888,7 +888,11 @@ class moodle_xhprofrun implements iXHProfRuns { if (@gzuncompress(base64_decode($rec->data)) === false) { return unserialize(base64_decode($rec->data)); } else { - return unserialize(gzuncompress(base64_decode($rec->data))); + $info = unserialize(gzuncompress(base64_decode($rec->data))); + + // We want to apply some transformations here, in order to reduce + // the information for some complex (too many levels) cases. + return $this->reduce_run_data($info); } } @@ -963,11 +967,142 @@ class moodle_xhprofrun implements iXHProfRuns { $this->url = $url; } - // Private API starts here + // Private API starts here. protected function sum_calls($sum, $data) { return $sum + $data['ct']; } + + /** + * Reduce the run data to a more manageable size. + * + * This removes from the run data all the entries that + * are matching a group of regular expressions. + * + * The main use is to remove all the calls between "__Mustache" + * functions, which don't provide any useful information and + * make the call-graph too complex to be handled. + * + * @param array $info The xhprof run data, original array. + * @return array The xhprof run data, reduced array. + */ + protected function reduce_run_data(array $info): array { + // Define which (regular expressions) we want to remove. Already escaped if needed to, please. + $toremove = [ + '__Mustache.*==>__Mustache.*', // All __Mustache to __Mustache calls. + ]; + // Build the regular expression to be used. + $regexp = '/^(' . implode('|', $toremove) . ')$/'; + + // Given that the keys of the array have the format "parent==>child" + // we want to rebuild the array with the same structure but + // topologically sorted (parents always before children). + // Note that we do this exclusively to guarantee that the + // second pass (see below) works properly in all cases because, + // without it, we may need to perform N (while loop) second passes. + $sorted = $this->xhprof_topo_sort($info); + + // To keep track of removed and remaining (child-parent) pairs. + $removed = []; + $remaining = []; + + // First pass, we are going to remove all the elements which + // both parent and child are __Mustache function calls. + foreach ($sorted as $key => $value) { + if (!str_contains($key, '==>')) { + $parent = 'NULL'; + $child = $key; + } else { + list ($parent, $child) = explode('==>', $key); // TODO: Consider caching this in a property. + } + + if (preg_match($regexp, $key)) { + unset($sorted[$key]); + $removed[$child][$parent] = true; + } else { + $remaining[$child][$parent] = true; + } + } + + // Second pass, we are going to remove all the elements which + // parent was removed by first pass and doesn't appear anymore + // as a child of anything (aka, they have become orphaned). + // Note, that thanks to the topological sorting, we can be sure + // one unique pass is enough. Without it, we may need to perform + // N (while loop) second passes. + foreach ($sorted as $key => $value) { + if (!str_contains($key, '==>')) { + $parent = 'NULL'; + $child = $key; + } else { + list ($parent, $child) = explode('==>', $key); // TODO: Consider caching this in a property. + } + + if (isset($removed[$parent]) && !isset($remaining[$parent])) { + unset($sorted[$key]); + $removed[$child][$parent] = true; + unset($remaining[$child][$parent]); + // If this was the last parent of this child, remove it completely from the remaining array. + if (empty($remaining[$child])) { + unset($remaining[$child]); + } + } + } + + // We are done, let's return the reduced array. + return $sorted; + } + + + /** + * Sort the xhprof run pseudo-topologically, so all parents are always before their children. + * + * Note that this is not a proper, complex, recursive topological sorting algorithm, returning + * nodes that later have to be converted back to xhprof "pairs" but, instead, does the specific + * work to get those parent==>child (2 levels only) "pairs" sorted (parents always before children). + * + * @param array $info The xhprof run data, original array. + * + * @return array The xhprof run data, sorted array. + */ + protected function xhprof_topo_sort(array $info): array { + $sorted = []; + $visited = []; + $remaining = $info; + do { + $newremaining = []; + foreach ($remaining as $key => $value) { + // If we already have visited this element, we can skip it. + if (isset($visited[$key])) { + continue; + } + if (!str_contains($key, '==>')) { + // It's a root element, we can add it to the sorted array. + $sorted[$key] = $info[$key]; + $visited[$key] = true; + } else { + list ($parent, $child) = explode('==>', $key); // TODO: Consider caching this in a property. + if (isset($visited[$parent])) { + // Parent already visited, we can add any children to the sorted array. + $sorted[$key] = $info[$key]; + $visited[$child] = true; + } else { + // Cannot add this yet, we need to wait for the parent. + $newremaining[$key] = $value; + } + } + } + // Protection against infinite loops. + if (count($remaining) === count($newremaining)) { + $remaining = []; // So we exit the do...while loop. + } else { + $remaining = $newremaining; // There is still work to do. + } + } while (count($remaining) > 0); + + // We are done, let's return the sorted array. + return $sorted; + } } /** From 8ef2974af1f435dd7797e3a88f18c0c07b37f21b Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Wed, 6 Sep 2023 21:15:39 +0200 Subject: [PATCH 2/3] MDL-79285 xhprof: Cover the new functions with unit tests This covers the 2 new functions with unit tests: - xhprof_topo_sort() - reduce_run_data() Note that the example graph used in the provider is the one shown in the issue to explain the reduction procedure. --- lib/tests/xhprof_test.php | 176 ++++++++++++++++++++++++++++++++++- lib/xhprof/xhprof_moodle.php | 6 +- 2 files changed, 174 insertions(+), 8 deletions(-) diff --git a/lib/tests/xhprof_test.php b/lib/tests/xhprof_test.php index dac3f2168be..60ccbf910db 100644 --- a/lib/tests/xhprof_test.php +++ b/lib/tests/xhprof_test.php @@ -26,12 +26,17 @@ namespace core; */ class xhprof_test extends \advanced_testcase { + public static function setUpBeforeClass(): void { + global $CFG; + require_once($CFG->libdir . '/xhprof/xhprof_moodle.php'); + } + /** * Data provider for string matches * * @return array */ - public function profiling_string_matches_provider() { + public static function profiling_string_matches_provider(): array { return [ ['/index.php', '/index.php', true], ['/some/dir/index.php', '/index.php', false], @@ -61,19 +66,180 @@ class xhprof_test extends \advanced_testcase { /** * Test the matching syntax * + * @covers ::profiling_string_matches * @dataProvider profiling_string_matches_provider * @param string $string * @param string $patterns * @param bool $expected */ public function test_profiling_string_matches($string, $patterns, $expected) { - - global $CFG; - require_once($CFG->libdir . '/xhprof/xhprof_moodle.php'); - $result = profiling_string_matches($string, $patterns); $this->assertSame($result, $expected); } + /** + * Data provider for both the topological sort and the data reduction tests. + * + * @return array + */ + public static function run_data_provider(): array { + // This data corresponds to the runs used as example @ MDL-79285. + return [ + 'sorted_case' => [ + 'rundata' => array_flip([ + 'A', + 'A==>B', + 'A==>C', + 'A==>__Mustache4', + 'B==>__Mustache1', + '__Mustache1==>__Mustache2', + '__Mustache4==>__Mustache2', + '__Mustache4==>E', + 'E==>F', + 'C==>F', + '__Mustache2==>F', + '__Mustache2==>D', + 'D==>__Mustache3', + '__Mustache3==>F', + ]), + 'expectations' => [ + 'topofirst' => 'A', + 'topolast' => '__Mustache3==>F', + 'topocount' => 14, + 'topoorder' => [ + // Before and after pairs to verify they are ordered. + ['before' => 'A==>C', 'after' => 'C==>F'], + ['before' => 'D==>__Mustache3', 'after' => '__Mustache3==>F'], + ], + 'reducecount' => 8, + 'reduceremoved' => [ + // Elements that will be removed by the reduction. + '__Mustache1==>__Mustache2', + '__Mustache4==>__Mustache2', + '__Mustache2==>F', + '__Mustache2==>D', + '__Mustache2==>D', + '__Mustache3==>F', + ], + ], + ], + 'unsorted_case' => [ + 'rundata' => array_flip([ + 'A==>__Mustache4', + '__Mustache3==>F', + 'A==>B', + 'A==>C', + 'B==>__Mustache1', + '__Mustache1==>__Mustache2', + '__Mustache4==>__Mustache2', + '__Mustache4==>E', + 'E==>F', + 'C==>F', + '__Mustache2==>F', + '__Mustache2==>D', + 'D==>__Mustache3', + 'A', + ]), + 'expectations' => [ + 'topofirst' => 'A', + 'topolast' => '__Mustache3==>F', + 'topocount' => 14, + 'topoorder' => [ + // Before and after pairs to verify they are ordered. + ['before' => 'A==>C', 'after' => 'C==>F'], + ['before' => 'D==>__Mustache3', 'after' => '__Mustache3==>F'], + ], + 'reducecount' => 8, + 'reduceremoved' => [ + // Elements that will be removed by the reduction. + '__Mustache1==>__Mustache2', + '__Mustache4==>__Mustache2', + '__Mustache2==>F', + '__Mustache2==>D', + '__Mustache2==>D', + '__Mustache3==>F', + ], + ], + ], + ]; + } + + /** + * Test that topologically sorting the run data works as expected + * + * @covers \moodle_xhprofrun::xhprof_topo_sort + * @dataProvider run_data_provider + * + * @param array $rundata The run data to be sorted. + * @param array $expectations The expected results. + */ + public function test_xhprof_topo_sort(array $rundata, array $expectations) { + // Make sure all the examples in the provider are the same size. + $this->assertSame($expectations['topocount'], count($rundata)); + + // Make moodle_xhprofrun::xhprof_topo_sort() accessible. + $reflection = new \ReflectionClass('\moodle_xhprofrun'); + $method = $reflection->getMethod('xhprof_topo_sort'); + $method->setAccessible(true); + // Sort the data. + $result = $method->invokeArgs(new \moodle_xhprofrun(), [$rundata]); + $this->assertIsArray($result); + $this->assertSame($expectations['topocount'], count($result)); + // Convert the array to a list of keys, so we can assert values by position. + $resultkeys = array_keys($result); + + // This is the elements that should be first. + $this->assertSame($expectations['topofirst'], $resultkeys[0]); + // This is the element that should be last. + $this->assertSame($expectations['topolast'], $resultkeys[$expectations['topocount'] - 1]); + // This relative ordering should be respected. + foreach ($expectations['topoorder'] as $order) { + // All the elements in the expectations should be present. + $this->assertArrayHasKey($order['before'], $result); + $this->assertArrayHasKey($order['after'], $result); + // And they should be in the correct relative order. + $this->assertGreaterThan( + array_search($order['before'], $resultkeys), + array_search($order['after'], $resultkeys) + ); + } + + // Final check, if we sort it again, nothing changes (it's already topologically sorted). + $result2 = $method->invokeArgs(new \moodle_xhprofrun(), [$result]); + $this->assertSame($result, $result2); + } + + /** + * Test that reducing the data complexity works as expected + * + * @covers \moodle_xhprofrun::reduce_run_data + * @dataProvider run_data_provider + * + * @param array $rundata The run data to be reduced. + * @param array $expectations The expected results. + */ + public function test_reduce_run_data(array $rundata, array $expectations) { + // Make sure that the expected keys that will be removed are present. + foreach ($expectations['reduceremoved'] as $key) { + $this->assertArrayHasKey($key, $rundata); + } + + // Make moodle_xhprofrun::reduce_run_data() accessible. + $reflection = new \ReflectionClass('\moodle_xhprofrun'); + $method = $reflection->getMethod('reduce_run_data'); + $method->setAccessible(true); + // Reduce the data. + $result = $method->invokeArgs(new \moodle_xhprofrun(), [$rundata]); + $this->assertIsArray($result); + $this->assertSame($expectations['reducecount'], count($result)); + // These have been the removed elements. + foreach ($expectations['reduceremoved'] as $key) { + $this->assertArrayNotHasKey($key, $result); + } + + // Final check, if we reduce it again, nothing changes (it's already reduced). + $result2 = $method->invokeArgs(new \moodle_xhprofrun(), [$result]); + $this->assertSame($result, $result2); + } } diff --git a/lib/xhprof/xhprof_moodle.php b/lib/xhprof/xhprof_moodle.php index cd33d3be037..5e5bc124d05 100644 --- a/lib/xhprof/xhprof_moodle.php +++ b/lib/xhprof/xhprof_moodle.php @@ -1013,7 +1013,7 @@ class moodle_xhprofrun implements iXHProfRuns { $parent = 'NULL'; $child = $key; } else { - list ($parent, $child) = explode('==>', $key); // TODO: Consider caching this in a property. + [$parent, $child] = explode('==>', $key); // TODO: Consider caching this in a property. } if (preg_match($regexp, $key)) { @@ -1035,7 +1035,7 @@ class moodle_xhprofrun implements iXHProfRuns { $parent = 'NULL'; $child = $key; } else { - list ($parent, $child) = explode('==>', $key); // TODO: Consider caching this in a property. + [$parent, $child] = explode('==>', $key); // TODO: Consider caching this in a property. } if (isset($removed[$parent]) && !isset($remaining[$parent])) { @@ -1081,7 +1081,7 @@ class moodle_xhprofrun implements iXHProfRuns { $sorted[$key] = $info[$key]; $visited[$key] = true; } else { - list ($parent, $child) = explode('==>', $key); // TODO: Consider caching this in a property. + [$parent, $child] = explode('==>', $key); // TODO: Consider caching this in a property. if (isset($visited[$parent])) { // Parent already visited, we can add any children to the sorted array. $sorted[$key] = $info[$key]; From 71bc684c41a26656ace3c576e87456d33d28b025 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Sun, 17 Sep 2023 23:06:01 +0200 Subject: [PATCH 3/3] MDL-79285 xhprof: Add support for optional "reducedata" parameter This new parameter / property will decide if we want to reduce the run data before processing it: - By default it will be disabled in table mode. - By default it will be enabled in graph mode. - The defaults can be changed by adding reducedata=[0|1] in the URLs - Once data reduction is enabled, it stays enabled while navigating within the xhprof reports. --- lib/xhprof/xhprof_html/callgraph.php | 1 + lib/xhprof/xhprof_html/index.php | 6 ++++++ lib/xhprof/xhprof_moodle.php | 16 ++++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/lib/xhprof/xhprof_html/callgraph.php b/lib/xhprof/xhprof_html/callgraph.php index 5076eab9ef5..a1415341974 100644 --- a/lib/xhprof/xhprof_html/callgraph.php +++ b/lib/xhprof/xhprof_html/callgraph.php @@ -90,6 +90,7 @@ if (!array_key_exists($type, $xhprof_legal_image_types)) { // Start moodle modification: use own XHProfRuns implementation. // $xhprof_runs_impl = new XHProfRuns_Default(); $xhprof_runs_impl = new moodle_xhprofrun(); +$xhprof_runs_impl->set_reducedata(xhprof_get_bool_param('reducedata', 1)); // Reduce data by default. // End moodle modification. if (!empty($run)) { diff --git a/lib/xhprof/xhprof_html/index.php b/lib/xhprof/xhprof_html/index.php index 80e7821fe92..4d29234ee7f 100644 --- a/lib/xhprof/xhprof_html/index.php +++ b/lib/xhprof/xhprof_html/index.php @@ -92,6 +92,12 @@ $vgbar = ' class="vgbar"'; // Start moodle modification: use own XHProfRuns implementation. // $xhprof_runs_impl = new XHProfRuns_Default(); $xhprof_runs_impl = new moodle_xhprofrun(); +$reducedata = xhprof_get_bool_param('reducedata', 0); // Don't reduce data by default. +$xhprof_runs_impl->set_reducedata($reducedata); +if ($reducedata) { + // We need to inject it, so we continue in "reduced data mode" all the time. + $params['reducedata'] = $reducedata; +} // End moodle modification. displayXHProfReport($xhprof_runs_impl, $params, $source, $run, $wts, diff --git a/lib/xhprof/xhprof_moodle.php b/lib/xhprof/xhprof_moodle.php index 5e5bc124d05..86c63a6d526 100644 --- a/lib/xhprof/xhprof_moodle.php +++ b/lib/xhprof/xhprof_moodle.php @@ -859,6 +859,9 @@ class moodle_xhprofrun implements iXHProfRuns { protected $totalmemory = 0; protected $timecreated = 0; + /** @var bool Decide if we want to reduce profiling data or no */ + protected bool $reducedata = false; + public function __construct() { $this->timecreated = time(); } @@ -889,6 +892,10 @@ class moodle_xhprofrun implements iXHProfRuns { return unserialize(base64_decode($rec->data)); } else { $info = unserialize(gzuncompress(base64_decode($rec->data))); + if (!$this->reducedata) { + // We want to return the full data. + return $info; + } // We want to apply some transformations here, in order to reduce // the information for some complex (too many levels) cases. @@ -967,6 +974,15 @@ class moodle_xhprofrun implements iXHProfRuns { $this->url = $url; } + /** + * Enable or disable reducing profiling data. + * + * @param bool $reducedata Decide if we want to reduce profiling data (true) or no (false). + */ + public function set_reducedata(bool $reducedata): void { + $this->reducedata = $reducedata; + } + // Private API starts here. protected function sum_calls($sum, $data) {