Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed issue #CT-652: system slowness when saving survey settings #3740

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Fixed issue #CT-652: system slowness when saving survey settings #3740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7faa12b
Dev: increase performance when saving survey settings
ptelu Feb 14, 2024
5291b55
Dev: performance improvement when updating question attributes as par…
ptelu Feb 14, 2024
613983c
Merge branch 'master' into optimisations_general_settings
ptelu Feb 15, 2024
6477d12
Dev: revert static for attribute provider
ptelu Feb 15, 2024
5d81b58
Dev: code style
ptelu Feb 15, 2024
153c3b7
Dev: StartProcessingGroup requires gseq not gid
ptelu Feb 19, 2024
074e810
Dev: performance improvement when updating question attributes as par…
ptelu Feb 19, 2024
c5c093e
WIP gseq fixes
ptelu Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion application/controllers/PrintanswersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ public function actionView($surveyid, $printableexport = false)
$oPDF->initAnswerPDF($aSurveyInfo, $aPdfLanguageSettings, Yii::app()->getConfig('sitename'), $sSurveyName, $sDefaultHeaderString);
LimeExpressionManager::StartProcessingPage(true); // means that all variables are on the same page
// Since all data are loaded, and don't need JavaScript, pretend all from Group 1
LimeExpressionManager::StartProcessingGroup(1, ($aSurveyInfo['anonymized'] != "N"), $iSurveyID);
LimeExpressionManager::StartProcessingGroup(0, ($aSurveyInfo['anonymized'] != "N"), $iSurveyID);
$aData['aSurveyInfo']['printPdf'] = 1;
$aData['aSurveyInfo']['include_content'] = 'printanswers';
Yii::app()->clientScript->registerPackage($oTemplate->sPackageName);
Expand Down
7 changes: 4 additions & 3 deletions application/controllers/QuestionAdministrationController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,9 +401,10 @@ public function getReorderData($oSurvey)
$groupData = [];
$initializedReplacementFields = false;
foreach ($groups as $iGID => $oGroup) {
$groupData[$iGID]['gid'] = $oGroup->gid;
$groupData[$iGID]['group_text'] = $oGroup->gid . ' ' . $oGroup->questiongroupl10ns[$baselang]->group_name;
LimeExpressionManager::StartProcessingGroup($oGroup->gid, false, $iSurveyID);
$gid = $oGroup->gid;
$groupData[$iGID]['gid'] = $gid;
$groupData[$iGID]['group_text'] = $gid . ' ' . $oGroup->questiongroupl10ns[$baselang]->group_name;
LimeExpressionManager::StartProcessingGroup(LimeExpressionManager::GetGroupSeq($gid), false, $iSurveyID);
if (!$initializedReplacementFields) {
templatereplace("{SITENAME}"); // Hack to ensure the EM sets values of LimeReplacementFields
$initializedReplacementFields = true;
Expand Down
5 changes: 3 additions & 2 deletions application/controllers/SurveyAdministrationController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2441,7 +2441,8 @@ public function actionCopy()
LimeExpressionManager::StartProcessingPage(true, true);
$aGrouplist = QuestionGroup::model()->findAllByAttributes(['sid' => $aImportResults['newsid']]);
foreach ($aGrouplist as $aGroup) {
LimeExpressionManager::StartProcessingGroup($aGroup['gid'], $oSurvey->anonymized != 'Y', $aImportResults['newsid']);
$gid = $aGroup->gid;
LimeExpressionManager::StartProcessingGroup(LimeExpressionManager::GetGroupSeq($gid), $oSurvey->anonymized !== 'Y', $aImportResults['newsid']);
LimeExpressionManager::FinishProcessingGroup();
}
LimeExpressionManager::FinishProcessingPage();
Expand Down Expand Up @@ -2679,7 +2680,7 @@ private function showReorderForm($iSurveyID)
foreach ($groups as $iGID => $oGroup) {
$groupData[$iGID]['gid'] = $oGroup->gid;
$groupData[$iGID]['group_text'] = $oGroup->gid . ' ' . $oGroup->questiongroupl10ns[$sBaseLanguage]->group_name;
LimeExpressionManager::StartProcessingGroup($oGroup->gid, false, $iSurveyID);
LimeExpressionManager::StartProcessingGroup(LimeExpressionManager::GetGroupSeq($oGroup->gid), false, $iSurveyID);
if (!$initializedReplacementFields) {
templatereplace("{SITENAME}"); // Hack to ensure the EM sets values of LimeReplacementFields
$initializedReplacementFields = true;
Expand Down
2 changes: 1 addition & 1 deletion application/controllers/admin/DataEntry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2112,7 +2112,7 @@ public function view($surveyid)
$aGroups = $survey->groups;
$aDataentryoutput = '';
foreach ($aGroups as $arGroup) {
LimeExpressionManager::StartProcessingGroup($arGroup->gid, ($thissurvey['anonymized'] != "N"), $surveyid);
LimeExpressionManager::StartProcessingGroup(LimeExpressionManager::GetGroupSeq($arGroup->gid), ($thissurvey['anonymized'] != "N"), $surveyid);

$aQuestions = $arGroup->questions;
$aDataentryoutput .= "\t<tr class='info'>\n"
Expand Down
52 changes: 35 additions & 17 deletions application/helpers/expressions/em_manager_helper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1085,6 +1085,7 @@ public static function ConvertConditionsToRelevance($surveyId, $qid = null)
*/
public static function UnitTestConvertConditionsToRelevance($surveyId = null, $qid = null)
{
LimeExpressionManager::SetDirtyFlag();
$LEM =& LimeExpressionManager::singleton();
return $LEM->ConvertConditionsToRelevance($surveyId, $qid);
}
Expand Down Expand Up @@ -3271,7 +3272,7 @@ private function _recursivelyFindAntecdentArrayFilters($qroot, $aflist, $afelist
* @return boolean|null - true if $fieldmap had been re-created, so ExpressionManager variables need to be re-set
* @todo Keep method as-is but factor out content to new class; add unit tests for class
*/
public function setVariableAndTokenMappingsForExpressionManager($surveyid, $forceRefresh = false, $anonymized = false)
public function setVariableAndTokenMappingsForExpressionManager($surveyid, $forceRefresh = false, $anonymized = false, $gid = 0)
{
if (isset($_SESSION['LEMforceRefresh'])) {
unset($_SESSION['LEMforceRefresh']);
Expand Down Expand Up @@ -3365,7 +3366,7 @@ public function setVariableAndTokenMappingsForExpressionManager($surveyid, $forc
$this->groupId2groupSeq[$aGroupInfo['gid']] = $aGroupInfo['group_order'];
}

$qattr = $this->getQuestionAttributesForEM($surveyid, 0, $_SESSION['LEMlang']);
$qattr = $this->getQuestionAttributesForEM($surveyid, 0, $_SESSION['LEMlang'], $gid);

$this->qattr = $qattr;

Expand Down Expand Up @@ -5629,7 +5630,7 @@ public function _ValidateGroup($groupSeq, $force = false)
$qInfo = $LEM->questionSeq2relevance[$groupSeqInfo['qstart']];
$gseq = $qInfo['gseq'];
$gid = $qInfo['gid'];
$LEM->StartProcessingGroup($gseq, $LEM->surveyOptions['anonymized'], $LEM->sid); // analyze the data we have about this group
$LEM->StartProcessingGroup(LimeExpressionManager::GetGroupSeq($gid), $LEM->surveyOptions['anonymized'], $LEM->sid); // analyze the data we have about this group

$grel = false; // assume irrelevant until find a relevant question
$ghidden = true; // assume hidden until find a non-hidden question. If there are no relevant questions on this page, $ghidden will stay true
Expand Down Expand Up @@ -6772,7 +6773,7 @@ public static function StartProcessingGroup($gseq = null, $anonymized = false, $
if (!is_null($gseq)) {
$LEM->currentGroupSeq = $gseq;
if (!is_null($surveyid)) {
$LEM->setVariableAndTokenMappingsForExpressionManager($surveyid, $forceRefresh, $anonymized);
$LEM->setVariableAndTokenMappingsForExpressionManager($surveyid, $forceRefresh, $anonymized, $LEM->gseq2info[$gseq]['gid']);
if ($gseq > $LEM->maxGroupSeq) {
$LEM->maxGroupSeq = $gseq;
}
Expand Down Expand Up @@ -7867,7 +7868,7 @@ public static function UnitTestProcessStringContainingExpressions()
$alltests[] = 'This line has a hidden script: <script type="text/javascript" language="Javascript">' . $javascript2 . '</script>';

LimeExpressionManager::StartProcessingPage();
LimeExpressionManager::StartProcessingGroup(1);
LimeExpressionManager::StartProcessingGroup(0);

$LEM =& LimeExpressionManager::singleton();
$LEM->tempVars = $vars;
Expand Down Expand Up @@ -7951,7 +7952,7 @@ public static function UnitTestRelevance()


LimeExpressionManager::StartProcessingPage(true);
LimeExpressionManager::StartProcessingGroup(1); // pretending this is group 1
LimeExpressionManager::StartProcessingGroup(0); // pretending this is group 1

// collect variables
$i = 0;
Expand Down Expand Up @@ -8233,16 +8234,14 @@ public static function UpgradeQuestionAttributes($changeDB = false, $iSurveyID =
* @param int|null $surveyid
* @param int|null $qid
* @param string|null $lang
* @param int $gid
*
* @return array
* @throws CException
* @throws EmCacheException
*/
private function getQuestionAttributesForEM($surveyid = 0, $qid = 0, $lang = '')
private function getQuestionAttributesForEM($surveyid = 0, $qid = 0, $lang = '', $gid = 0)
{
$cacheKey = 'getQuestionAttributesForEM_' . $surveyid . '_' . $qid . '_' . $lang;
$value = EmCacheHelper::get($cacheKey);
if ($value !== false) {
return $value;
}

// Fix old param (NULL)
if (is_null($surveyid)) {
$surveyid = 0;
Expand All @@ -8253,6 +8252,15 @@ private function getQuestionAttributesForEM($surveyid = 0, $qid = 0, $lang = '')
if (is_null($lang)) {
$lang = '';
}
if (is_null($gid) && $gid < 0) {
$gid = 0;
}
$cacheKey = 'getQuestionAttributesForEM_' . $surveyid . '_' . $gid . '_' . $qid . '_' . $lang;
$value = EmCacheHelper::get($cacheKey);
if ($value !== false) {
return $value;
}

// Fill $lang if possible
if (!$lang && isset($_SESSION['LEMlang'])) {
$lang = $_SESSION['LEMlang'];
Expand All @@ -8277,14 +8285,24 @@ private function getQuestionAttributesForEM($surveyid = 0, $qid = 0, $lang = '')
'params' => [':qid' => $qid]
]
);
} elseif ($gid !== 0 && $surveyid) {
$oQids = Question::model()->findAll(
[
'select' => 'qid',
'group' => 'qid',
'distinct' => true,
'condition' => "sid=:sid and parent_qid=0 and gid=:gid",
'params' => [':sid' => $surveyid, ':gid' => $gid]
]
);
} elseif ($surveyid) {
$oQids = Question::model()->findAll(
[
'select' => 'qid',
'group' => 'qid',
'distinct' => true,
'select' => 'qid',
'group' => 'qid',
'distinct' => true,
'condition' => "sid=:sid and parent_qid=0",
'params' => [':sid' => $surveyid]
'params' => [':sid' => $surveyid]
]
);
} else {
Expand Down
13 changes: 11 additions & 2 deletions application/models/QuestionTheme.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,8 +835,16 @@ public static function getAnswerColumnDefinition($name, $type)
*/
public static function getQuestionXMLPathForBaseType($type)
{
/** @var QuestionTheme|null */
$questionTheme = QuestionTheme::model()->findByAttributes([], 'question_type = :question_type AND extends = :extends', ['question_type' => $type, 'extends' => '']);
static $questionThemeCache = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike to use Cache for Static var … Static is not caching ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah makes sense, cant use it everywhere php will run out of memory eventually when we use it to much. Will look at yii's caching

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently : it's a static var :). I don't ask to put it in cache (maybe in future), just more clear name :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also propose the creation of a protected static $questionThemeCache = []; data member to make the file more readable and the cache more accessible.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected static $questionThemeStatic = [] ;)

if (!array_key_exists($type, $questionThemeCache)) {
$questionThemeCache[$type] = self::model()->findByAttributes(
[],
'question_type = :question_type AND extends = :extends',
['question_type' => $type, 'extends' => '']
);
}
$questionTheme = $questionThemeCache[$type];

if (empty($questionTheme)) {
throw new \CException("The Database definition for Questiontype: " . $type . " is missing");
}
Expand Down Expand Up @@ -1017,6 +1025,7 @@ public static function getAdditionalAttrFromExtendedTheme($sQuestionThemeName, $
if (\PHP_VERSION_ID < 80000) {
libxml_disable_entity_loader(false);
}

$questionTheme = QuestionTheme::model()->findByAttributes([], 'name = :name AND extends = :extends', ['name' => $sQuestionThemeName, 'extends' => $type]);
if ($questionTheme !== null) {
$xml_config = simplexml_load_file($questionTheme->getXmlPath() . '/config.xml');
Expand Down
3 changes: 2 additions & 1 deletion application/models/services/Proxy/ProxyExpressionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ public function reset($surveyId)
$aGrouplist = $this->modelQuestionGroup
->findAllByAttributes(['sid' => $surveyId]);
foreach ($aGrouplist as $aGroup) {
$gid = $aGroup['gid'];
LimeExpressionManager::StartProcessingGroup(
$aGroup['gid'],
LimeExpressionManager::GetGroupSeq($gid),
$survey->anonymized != 'Y',
$surveyId
);
Expand Down
2 changes: 2 additions & 0 deletions application/models/services/ThemeQuestionAttributeProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

namespace LimeSurvey\Models\Services;

use QuestionTheme;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use QuestionTheme here?


/**
* Provides question attribute definitions from question themes
*/
Expand Down