surveyjs / survey-library

Free JavaScript form builder library with integration for React, Angular, Vue, jQuery, and Knockout.
https://surveyjs.io/form-library
MIT License
4.09k stars 795 forks source link

[idea] Simplify & improve performance of expressions and conditions #7697

Open SamMousa opened 7 months ago

SamMousa commented 7 months ago

Are you requesting a feature, reporting a bug or asking a question?

None of the above; I 'm proposing an idea.

What is the current behavior?

Whenever a value changes, for example via survey.setVariable('name', 'newValue') surveyJS inefficiently recalculates a whole bunch of stuff. For example it always recalculates all calculated values, iterates over all pages etc:

private runConditionsCore(properties: any) {
    var pages = this.pages;
    for (var i = 0; i < this.calculatedValues.length; i++) {
      this.calculatedValues[i].resetCalculation();
    }
    for (var i = 0; i < this.calculatedValues.length; i++) {
      this.calculatedValues[i].doCalculation(
        this.calculatedValues,
        this.conditionValues,
        properties
      );
    }
    super.runConditionCore(this.conditionValues, properties);
    for (let i = 0; i < pages.length; i++) {
      pages[i].runCondition(this.conditionValues, properties);
    }
  }

Not only is this inefficient, it also requires a top down awareness of all things that might need updating.

Bottom up approach using observables

An observable is similar to a promise, but instead of one value it can have multiple values. Using a library like rxjs we have advanced operators that allow for automatic optimization. The automatic optimization happens due to the way operators work. For example if we have this code:

const survey: SurveyModel = new SurveyModel(...);

const oQuestion1 = new BehaviorSubject(survey.getValue('question1');
const oQuestion2 = new BehaviorSubject(survey.getValue('question2');

survey.onValueChanged((survey: SurveyModel) => {
    // We ignore the fact that we have the question, name and value available as other arguments.
    oQuestion1.next(survey.getValue('question1'));
    oQuestion2.next(survey.getValue('question2'));
});

// Now we have an expression like this {question1} + {question2}:
oExpression1 = combineLatest([
    oQuestion1.pipe(distinctUntilChanged()),
    oQuestion2.pipe(distinctUntilChanged()),
]).pipe(map([question1, question2] => question1 + question2);

The cool thing about these is that they are only evaluated when actually used. So if no one is currently using oExpression1, because, for example, the page is not visible, the value will not be calculated. Also because since the dependencies are managed locally by each observable, we stop the change as early as possible not wasting time recalculating an expression if the value didn't actually change, or if a value that we don't care about changed.

This approach could simplify and improve many of the survey engine logic, and the great thing is, it doesn't have to replace everything at once. It could be added incrementally under the hood and externally no one would notice.

Other areas where observables might simplify life

Localizations could use a similar pattern, where you create an observable that combines the survey current language (as an observable) with the string configuration (as an observable) and its current value in the current locale is exposed as an observable. Rendering wise this integrates very easily with all modern frameworks, so much so that frameworks will often unsubscribe from observable when their content is not visible; further optimizing performance.

andrewtelnov commented 7 months ago

@SamMousa Unfortunately it doesn't work in several cases, when somebody using survey variables in their custom functions. We can add ability to lock condition calculation and then unlock it with optionally (default) re-calculating all values. Will it work for you?

Thank you, Andrew

SamMousa commented 7 months ago

Unfortunately it doesn't work in several cases, when somebody using survey variables in their custom functions.

I've been thinking on that as well. You indeed can't know if a custom function result changes if it directly accesses the survey. On the other hand if it only accesses variables like this: myCustomFunc({q1},{q2}) you can for sure no it. That said, this is about optimization, you can still make this work for those function and worst case it'll perform similar to current performance.

  1. Detect if it uses custom functions, if so use the whole survey data as a source for trigger updates.
  2. If it only uses builtin functions and operators detect which variables are passed in and use those.

We can add ability to lock condition calculation and then unlock it with optionally (default) re-calculating all values. Will it work for you?

No it won't, the issue is that I don't know the survey structure in advance. Furthermore I prefer to spend time improving the surveyjs core library versus creating custom code in my project to handle edge cases.

I'll let this simmer a bit more and see if I can work on a basic PR to optimize just 1 case for example. If the code looks clean and the idea works it might inspire you!

SamMousa commented 5 months ago

Since I needed to know when a calculated text changes for #7945 I implemented something here: https://gist.github.com/SamMousa/2ba22cf6e7300e70bfda97d080e016b5

(This is not finished, for now it supports just data values not calculatedValues or variables)

The cool thing is that it uses RxJS observables, is efficiently updated and lives outside of the SurveyJS object hierarchy.

Usage:

import calculatedText from '...';
import type { SurveyModel } from 'survey-core';

const survey: SurveyModel = getSurveySomehow();

const someText: Observable<string> = calculatedText(survey, "some nice text that supports values like {q1} and {q2}.");

someText.subscribe((value: string) => {
console.log("Your text changed to: ", value);
});

Most frontend frameworks support observables natively or with very small adapter code. Furthermore the way observables work means that they can do smart things like not calculate stuff when nobody is subscribed. Frameworks will auto unsubscribe from observables when the views using them are currently not visible etc.

SamMousa commented 1 month ago

Going further on this:

I've created 2 helpers:

import { Observable, distinctUntilChanged, shareReplay } from 'rxjs';
import type { SurveyModel, ValueChangedEvent } from 'survey-core';

export default function observableValue<T>(subject: SurveyModel, valueName: string): Observable<T> {
    const o = new Observable<T>((subscriber) => {
        const changedHandler = (_survey: SurveyModel, { name, value }: ValueChangedEvent) => {
            if (name === valueName) {
                subscriber.next(value);
            }
        };
        subject.onValueChanged.add(changedHandler);
        subscriber.next(subject.getValue(valueName));

        return () => {
            subject.onValueChanged.remove(changedHandler);
        };
    });

    return o.pipe(distinctUntilChanged(), shareReplay({ bufferSize: 1, refCount: true }));
}

This helper creates an observable that emits survey values when they change.

import { Observable, combineLatest, map, shareReplay, distinctUntilChanged } from 'rxjs';
import type { SurveyModel } from 'survey-core';
import observableValue from './observableValue';
import * as surveyCore from 'survey-core';
const { ConditionsParser, ProcessValue } = surveyCore;

export default function observableExpression<T = unknown>(
    survey: SurveyModel,
    expression: string
): Observable<T> {
    const p = new ConditionsParser();
    const op = p.parseExpression(expression);

    const o = new Observable<T>((subscriber) => {
        const dependentVariables: Array<string> = [];
        op.setVariables(dependentVariables);

        // Create an observable for each value that this expression depends on.
        const values = dependentVariables.map((v) => observableValue(survey, v));

        // Combine all watched values into a single observable that contains the array of values.
        return combineLatest(values)
            .pipe(
                // Map the array of values to a dictionary / hash
                map((values) => {
                    const dict: Record<string, unknown> = {};
                    for (let i = 0; i < values.length; i++) {
                        dict[dependentVariables[i]] = values[i];
                    }
                    return dict;
                }),
                // Evaluate the expression
                map((valueHash) => {
                    const pv = new ProcessValue();
                    pv.values = valueHash;
                    pv.properties = {
                        survey
                    };
                    return op.evaluate(pv);
                })
            )
            .subscribe(subscriber);
    });
    return o.pipe(distinctUntilChanged(), shareReplay({ bufferSize: 1, refCount: true }));
}

This second helper uses the expression parser from SurveyJS to parse an expression.

The nice thing about this approach is that it requires no manual dependency tracking. Every part of the core SurveyJS code is littered callbacks, object properties that serve as some kind of cache, booleans that track in what state of evaluation we are etc.

For example on every value change, for EACH calculated value in the survey:

This comes at a performance cost, which is not too relevant. But it also comes with a code readability cost. Half the code in CalculatedValue is about managing the updating of the expression result.

What could be a relevant performance issue is the fact that these calculated values are created greedily. This is because there's no clean way to decide who actually needs the calculated values. Due to the way observables work, naturally if no one is listening it will not do the calculations.

These helpers allow me to listen to each visibleIf expression and when its result changes I can do the minimum required work to update the UI. (in this case, hide an object on a FabricJS canvas)