From 7fdbb59c3072aab29a1e6df9a901b7e0d20443ae Mon Sep 17 00:00:00 2001 From: Eric Espie Date: Wed, 13 Sep 2023 15:58:26 +0200 Subject: [PATCH] =?UTF-8?q?N=C2=B06716=20-=20High=20memory=20Consomption?= =?UTF-8?q?=20and=20performance=20issue?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/dbobject.class.php | 105 ++++++++++++++++-- pages/schema.php | 13 ++- setup/compiler.class.inc.php | 7 +- sources/Service/Events/EventService.php | 13 ++- .../unitary-tests/core/CRUDEventTest.php | 19 ++++ .../MockDBObjectWithCRUDEventListener.php | 44 ++++++++ .../unitary-tests/core/DBObjectTest.php | 36 ++++++ 7 files changed, 219 insertions(+), 18 deletions(-) create mode 100644 tests/php-unit-tests/unitary-tests/core/DBObject/MockDBObjectWithCRUDEventListener.php diff --git a/core/dbobject.class.php b/core/dbobject.class.php index 5b1e3dd18..8466d4a66 100644 --- a/core/dbobject.class.php +++ b/core/dbobject.class.php @@ -6,7 +6,9 @@ use Combodo\iTop\Core\MetaModel\FriendlyNameType; use Combodo\iTop\Service\Events\EventData; +use Combodo\iTop\Service\Events\EventException; use Combodo\iTop\Service\Events\EventService; +use Combodo\iTop\Service\Events\EventServiceLog; use Combodo\iTop\Service\TemporaryObjects\TemporaryObjectManager; /** @@ -203,6 +205,8 @@ abstract class DBObject implements iDisplay const MAX_UPDATE_LOOP_COUNT = 10; + private $aEventListeners = []; + /** * DBObject constructor. * @@ -255,6 +259,10 @@ abstract class DBObject implements iDisplay $this->RegisterEventListeners(); } + /** + * @see RegisterCRUDListener + * @see EventService::RegisterListener() + */ protected function RegisterEventListeners() { } @@ -6165,6 +6173,51 @@ abstract class DBObject implements iDisplay return OPT_ATT_NORMAL; } + public final function GetListeners(): array + { + $aListeners = []; + foreach ($this->aEventListeners as $aEventListener) { + $aListeners = array_merge($aListeners, $aEventListener); + } + return $aListeners; + } + + /** + * Register a callback for a specific event. The method to call will be saved in the object instance itself whereas calling {@see EventService::RegisterListener()} would + * save a callable (thus the method name AND the whole DBObject instance) + * + * @param string $sEvent corresponding event + * @param string $callback The callback method to call + * @param float $fPriority optional priority for callback order + * @param string $sModuleId + * + * @see EventService::RegisterListener() + * + * @since 3.1.0-3 3.1.1 3.2.0 N°6716 + */ + final protected function RegisterCRUDListener(string $sEvent, string $callback, float $fPriority = 0.0, string $sModuleId = '') + { + $aEventCallbacks = $this->aEventListeners[$sEvent] ?? []; + + $aEventCallbacks[] = array( + 'event' => $sEvent, + 'callback' => $callback, + 'priority' => $fPriority, + 'module' => $sModuleId, + ); + usort($aEventCallbacks, function ($a, $b) { + $fPriorityA = $a['priority']; + $fPriorityB = $b['priority']; + if ($fPriorityA == $fPriorityB) { + return 0; + } + + return ($fPriorityA < $fPriorityB) ? -1 : 1; + }); + + $this->aEventListeners[$sEvent] = $aEventCallbacks; + } + /** * @param string $sEvent * @param array $aEventData @@ -6176,15 +6229,53 @@ abstract class DBObject implements iDisplay */ public function FireEvent(string $sEvent, array $aEventData = array()): void { - if (EventService::IsEventRegistered($sEvent)) { - $aEventData['debug_info'] = 'from: '.get_class($this).':'.$this->GetKey(); - $aEventData['object'] = $this; - $aEventSources = [$this->m_sObjectUniqId]; - foreach (MetaModel::EnumParentClasses(get_class($this), ENUM_PARENT_CLASSES_ALL, false) as $sClass) { - $aEventSources[] = $sClass; + $aEventData['debug_info'] = 'from: '.get_class($this).':'.$this->GetKey(); + $aEventData['object'] = $this; + + // Call local listeners first + $aEventCallbacks = $this->aEventListeners[$sEvent] ?? []; + $oFirstException = null; + $sFirstExceptionMessage = ''; + foreach ($aEventCallbacks as $aEventCallback) { + $oKPI = new ExecutionKPI(); + $sCallback = $aEventCallback['callback']; + if (!method_exists($this, $sCallback)) { + EventServiceLog::Error("Callback '".get_class($this).":$sCallback' does not exist"); + continue; + } + EventServiceLog::Debug("Fire event '$sEvent' calling '".get_class($this).":$sCallback'"); + try { + call_user_func([$this, $sCallback], new EventData($sEvent, null, $aEventData)); + } + catch (EventException $e) { + EventServiceLog::Error("Event '$sEvent' for '$sCallback'} failed with blocking error: ".$e->getMessage()); + throw $e; + } + catch (Exception $e) { + $sMessage = "Event '$sEvent' for '$sCallback'} failed with non-blocking error: ".$e->getMessage(); + EventServiceLog::Error($sMessage); + if (is_null($oFirstException)) { + $sFirstExceptionMessage = $sMessage; + $oFirstException = $e; + } + } + finally { + $oKPI->ComputeStats('FireEvent', $sEvent); } - EventService::FireEvent(new EventData($sEvent, $aEventSources, $aEventData)); } + if (!is_null($oFirstException)) { + throw new Exception($sFirstExceptionMessage, $oFirstException->getCode(), $oFirstException); + } + + // Call global event listeners + if (!EventService::IsEventRegistered($sEvent)) { + return; + } + $aEventSources = []; + foreach (MetaModel::EnumParentClasses(get_class($this), ENUM_PARENT_CLASSES_ALL, false) as $sClass) { + $aEventSources[] = $sClass; + } + EventService::FireEvent(new EventData($sEvent, $aEventSources, $aEventData)); } ////////////////// diff --git a/pages/schema.php b/pages/schema.php index 170553188..a12fe92ee 100644 --- a/pages/schema.php +++ b/pages/schema.php @@ -290,7 +290,6 @@ function DisplayEvents(WebPage $oPage, $sClass) foreach (MetaModel::EnumChildClasses($sClass, ENUM_CHILD_CLASSES_ALL) as $sChildClass) { if (!MetaModel::IsAbstract($sChildClass)) { $oObject = MetaModel::NewObject($sChildClass); - $aSources[] = $oObject->GetObjectUniqId(); break; } } @@ -299,7 +298,6 @@ function DisplayEvents(WebPage $oPage, $sClass) } } else { $oObject = MetaModel::NewObject($sClass); - $aSources[] = $oObject->GetObjectUniqId(); foreach (MetaModel::EnumParentClasses($sClass, ENUM_PARENT_CLASSES_ALL, false) as $sParentClass) { $aSources[] = $sParentClass; } @@ -320,12 +318,19 @@ function DisplayEvents(WebPage $oPage, $sClass) }); $aColumns = [ 'event' => ['label' => Dict::S('UI:Schema:Events:Event')], - 'listener' => ['label' => Dict::S('UI:Schema:Events:Listener')], + 'callback' => ['label' => Dict::S('UI:Schema:Events:Listener')], 'priority' => ['label' => Dict::S('UI:Schema:Events:Rank')], 'module' => ['label' => Dict::S('UI:Schema:Events:Module')], ]; + // Get the object listeners first $aRows = []; $oReflectionClass = new ReflectionClass($sClass); + if ($oReflectionClass->isInstantiable()) { + /** @var DBObject $oClass */ + $oClass = new $sClass(); + $aRows = $oClass->GetListeners(); + } + foreach ($aListeners as $aListener) { if (is_object($aListener['callback'][0])) { $sListenerClass = $sClass; @@ -343,7 +348,7 @@ function DisplayEvents(WebPage $oPage, $sClass) } $aRows[] = [ 'event' => $aListener['event'], - 'listener' => $sListener, + 'callback' => $sListener, 'priority' => $aListener['priority'], 'module' => $aListener['module'], ]; diff --git a/setup/compiler.class.inc.php b/setup/compiler.class.inc.php index e25d851b8..1959ea186 100644 --- a/setup/compiler.class.inc.php +++ b/setup/compiler.class.inc.php @@ -1449,17 +1449,12 @@ EOF } $sMethods .= "\n $sCallbackFct\n\n"; } - if (strpos($sCallback, '::') === false) { - $sEventListener = '[$this, \''.$sCallback.'\']'; - } else { - $sEventListener = "'$sCallback'"; - } $sListenerRank = (float)($oListener->GetChildText('rank', '0')); $sEvents .= <<m_sObjectUniqId, [], null, $sListenerRank, '$sModuleRelativeDir'); + \$this->RegisterCRUDListener("$sEventName", '$sCallback', $sListenerRank, '$sModuleRelativeDir'); PHP; } } diff --git a/sources/Service/Events/EventService.php b/sources/Service/Events/EventService.php index 072b77473..4611787d5 100644 --- a/sources/Service/Events/EventService.php +++ b/sources/Service/Events/EventService.php @@ -10,6 +10,7 @@ use Closure; use Combodo\iTop\Service\Events\Description\EventDescription; use ContextTag; use CoreException; +use DBObject; use Exception; use ExecutionKPI; use ReflectionClass; @@ -53,6 +54,12 @@ final class EventService /** * Register a callback for a specific event * + * **Warning** : be ultra careful on memory footprint ! each callback will be saved in {@see aEventListeners}, and a callback is + * made of the whole object instance and the method name ({@link https://www.php.net/manual/en/language.types.callable.php}). + * For example to register on DBObject instances, you should better use {@see DBObject::RegisterCRUDListener()} + * + * @uses aEventListeners + * * @api * @param string $sEvent corresponding event * @param callable $callback The callback to call @@ -61,8 +68,12 @@ final class EventService * @param array|string|null $context context filter * @param float $fPriority optional priority for callback order * - * @return string Id of the registration + * @return string registration identifier * + * @see DBObject::RegisterCRUDListener() to register in DBObject instances instead, to reduce memory footprint (callback saving) + * + * @since 3.1.0 method creation + * @since 3.1.0-3 3.1.1 3.2.0 N°6716 PHPDoc change to warn on memory footprint, and {@see DBObject::RegisterCRUDListener()} alternative */ public static function RegisterListener(string $sEvent, callable $callback, $sEventSource = null, array $aCallbackData = [], $context = null, float $fPriority = 0.0, $sModuleId = ''): string { diff --git a/tests/php-unit-tests/unitary-tests/core/CRUDEventTest.php b/tests/php-unit-tests/unitary-tests/core/CRUDEventTest.php index 95dc968db..aea1cbb52 100644 --- a/tests/php-unit-tests/unitary-tests/core/CRUDEventTest.php +++ b/tests/php-unit-tests/unitary-tests/core/CRUDEventTest.php @@ -12,6 +12,7 @@ use Combodo\iTop\Test\UnitTest\ItopDataTestCase; use ContactType; use CoreException; use DBObject; +use DBObject\MockDBObjectWithCRUDEventListener; use DBObjectSet; use DBSearch; use lnkPersonToTeam; @@ -521,6 +522,24 @@ class CRUDEventTest extends ItopDataTestCase $this->assertEquals(2, self::$aEventCalls[EVENT_DB_LINKS_CHANGED]); } + + // Tests with MockDBObject + public function testFireCRUDEvent() + { + $this->RequireOnceUnitTestFile('DBObject/MockDBObjectWithCRUDEventListener.php'); + + // For Metamodel list of classes + MockDBObjectWithCRUDEventListener::Init(); + $oDBObject = new MockDBObjectWithCRUDEventListener(); + $oDBObject2 = new MockDBObjectWithCRUDEventListener(); + + $oDBObject->FireEvent(MockDBObjectWithCRUDEventListener::TEST_EVENT); + + $this->assertNotNull($oDBObject->oEventDataReceived); + $this->assertNull($oDBObject2->oEventDataReceived); + + //echo($oDBObject->oEventDataReceived->Get('debug_info')); + } } /** diff --git a/tests/php-unit-tests/unitary-tests/core/DBObject/MockDBObjectWithCRUDEventListener.php b/tests/php-unit-tests/unitary-tests/core/DBObject/MockDBObjectWithCRUDEventListener.php new file mode 100644 index 000000000..f81ce637c --- /dev/null +++ b/tests/php-unit-tests/unitary-tests/core/DBObject/MockDBObjectWithCRUDEventListener.php @@ -0,0 +1,44 @@ + 'bizmodel, searchable', + 'key_type' => 'autoincrement', + 'name_attcode' => '', + 'state_attcode' => '', + 'reconc_keys' => [], + 'db_table' => 'priv_unit_tests_mock', + 'db_key_field' => 'id', + 'db_finalclass_field' => '', + 'display_template' => '', + 'indexes' => [], + ); + MetaModel::Init_Params($aParams); + } + + protected function RegisterEventListeners() + { + $this->RegisterCRUDListener(self::TEST_EVENT, 'TestEventCallback', 0, 'unit-test'); + } + + public function TestEventCallback(EventData $oEventData) + { + $this->oEventDataReceived = $oEventData; + } +} \ No newline at end of file diff --git a/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php b/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php index 23e9bd592..c8504ee7f 100644 --- a/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php +++ b/tests/php-unit-tests/unitary-tests/core/DBObjectTest.php @@ -897,4 +897,40 @@ class DBObjectTest extends ItopDataTestCase return $oPerson; } + /** + * @since 3.1.0-3 3.1.1 3.2.0 N°6716 test creation + */ + public function testConstructorMemoryFootprint():void + { + $idx = 0; + $fStart = microtime(true); + $fStartLoop = $fStart; + $iInitialPeak = 0; + $iMaxAllowedMemoryIncrease = 1 * 1024 * 1024; + + for ($i = 0; $i < 5000; $i++) { + /** @noinspection PhpUnusedLocalVariableInspection We intentionally use a reference that will disappear on each loop */ + $oPerson = new \Person(); + if (0 == ($idx % 100)) { + $fDuration = microtime(true) - $fStartLoop; + $iMemoryPeakUsage = memory_get_peak_usage(); + if ($iInitialPeak === 0) { + $iInitialPeak = $iMemoryPeakUsage; + $sInitialPeak = \utils::BytesToFriendlyFormat($iInitialPeak, 4); + } + + $sCurrPeak = \utils::BytesToFriendlyFormat($iMemoryPeakUsage, 4); + echo "$idx ".sprintf('%.1f ms', $fDuration * 1000)." - Peak Memory Usage: $sCurrPeak\n"; + + $this->assertTrue(($iMemoryPeakUsage - $iInitialPeak) <= $iMaxAllowedMemoryIncrease , "Peak memory changed from $sInitialPeak to $sCurrPeak after $i loops"); + + $fStartLoop = microtime(true); + } + $idx++; + } + + $fTotalDuration = microtime(true) - $fStart; + echo 'Total duration: '.sprintf('%.3f s', $fTotalDuration)."\n\n"; + } + }