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.

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

MW-1026: Avoiding session reset upon database preview #3745

Merged
merged 1 commit into from Feb 19, 2024

Conversation

lajosarpad
Copy link
Contributor

This is a performance patch, a direct follow-up of my chat with @Shnoulle about session reset in setVariableAndTokenMappingsForExpressionManager being executed too often, slowing down the entire system. According to my tests, there are cases when we set the preview mode to 'database' and therefore we reset for each group because of the criteria at

} elseif ($forceRefresh === false && !empty($this->knownVars) && !$this->sPreviewMode) {

We can see that if the preview is not equivalent to false, then we end up doing the entirety of the reset for each and every call, so when we loop the groups at

LimeExpressionManager::StartProcessingGroup(
force refresh being false, but at
$LEM->setVariableAndTokenMappingsForExpressionManager($_SESSION['LEMsid']);
which is being executed for each group we already have the preview mode being set to 'database' because of
LimeExpressionManager::SetPreviewMode('database');

so we do a reset for each and every group and that eats up all our memory and time resources, creating spikes.

We will need to check other use-cases of this feature as well whether we have similar preview mode sets that slow down the entire system.

Measurements:

Survey1 Text Element Save 1 15.49s - 3.25
Survey1 Reload: 1.85 - 0.34s
Survey1 Start: 2.62s - 1.94s
Survey2 Reload: 11.56s -> 0.81s
Survey2 Text Element Save 4.3 minutes - 40.68s
Survey2 Start: 21.6s - 20 s

@Shnoulle
Copy link
Collaborator

Shnoulle commented Feb 19, 2024

Seems really great for improvement, we can do the same for previewmode == 'logicfile' (unsure on the code here).

But a better improvement can be : add a var to know if it's already done ?

Something like, using global var (then the var don't enter in EM)

    public function setVariableAndTokenMappingsForExpressionManager($surveyid, $forceRefresh = false, $anonymized = false)
    {
        if (isset($_SESSION['LEMforceRefresh'])) {
            unset($_SESSION['LEMforceRefresh']);
            $forceRefresh = true;
        } elseif ($forceRefresh === false && !empty($this->knownVars) && !$this->sPreviewMode && !App()->getConfig('FreshVariableAndTokenMappingsForExpressionManager')) {
            return false;   // means that those variables have been cached and no changes needed
        }
        /* Set this variable is refreshed */
        App()->setConfig('FreshVariableAndTokenMappingsForExpressionManager') = true;
        // Here we do all action : refresh or create VariableAndTokenMappingsForExpressionManager
    }

@c-schmitz c-schmitz merged commit ae9bed5 into master Feb 19, 2024
20 checks passed
@c-schmitz c-schmitz deleted the bug/MW-1026-prevent-reset-when-unnecessary branch February 19, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants