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

Schema#addChildCheck leads to performance problems #15834

Closed
scofalik opened this issue Feb 12, 2024 · 1 comment · Fixed by #15959
Closed

Schema#addChildCheck leads to performance problems #15834

scofalik opened this issue Feb 12, 2024 · 1 comment · Fixed by #15959
Labels
domain:performance This issue reports a problem or potential improvement regarding editor performance. package:engine support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

📝 Provide a description of the improvement

Currently, we can use Schema#addChildCheck() to add custom callback that can affect whether some node is allowed at given place in model structure. The callbacks were meant to be flexible, you can check anything and as you wish in it. Same for Schema#addAttributeCheck().

Unfortunately, this can lead to performance problems if you start using many of them. Why?

Schema is checked very frequently by multiple features. Most commands check either whether it can insert a child at given place or whether it is possible to apply attribute for given selection. This is refreshed after every selection change and model change. With a rich set of plugins, you may have as many as 10-15 checks every keystroke. When multiple callbacks like this are added, this can become 50-100 callbacks each keystroke, and you may start to see downgrade in the editor responsiveness, especially on slower machines.

This is of course not necessary. There's no need for features A,B,C,D... to check whether feature X complies with schema. But since these are very simple callbacks, they are fired each time the schema checked by each feature.

However, we use declarative schema so much, how come it does not affect performance?

Well, declarative schema is checked only for a specified element/attribute. Schema#checkChild and Schema#checkAttribute gets a definition for specified element/attribute and only checks it. As a result feature X checks only feature X rules.

We should try to limit how many custom callbacks are called after each model update.

First, we should add additional parameter to addChildCheck() and addAttributeCheck() that will specify for which element/attribute this callback is declared. We can have this optional to still allow for very general checks but we would assume that these are used very rarely, for complex custom plugins, and maybe there's one or two such callbacks in the editor at most.

We should try to keep checkChild and checkAttribute events for backward compatibility but it might be that the arguments for these events will change, which will affect all addChildCheck() uses. But... as I look at it know, it shouldn't change, as we normalize the arguments in the highest priority listener:

this.on( 'checkChild', ( evt, args ) => {
	args[ 0 ] = new SchemaContext( args[ 0 ] );
	args[ 1 ] = this.getDefinition( args[ 1 ] );
}, { priority: 'highest' } );

So, after adding the extra parameter, we have two ways of solving the performance issue.

First -- instead of simply adding the callback as an event listener, we would save it in an internal map in Schema. Then, when checkChild() is called, before doing default declarative check, we would go through all the registered callbacks for given definition.

Second -- we could use namespaced events. So, if I check schema for paragraph, then checkChild:paragraph is called. In this solution, we don't need to change much how addChildCheck() works. We will simply add callbacks to the namespaced events. However, checkChild() cannot be a decorated method any longer. Instead, it would have to be like this:

public checkChild( context, def ) {
    normalize(); // What happens in highest priority listener now.

    if ( !def ) {
        return false; // As in `checkChild()` now.
    }

    // The default check in `_checkContextMatch()` must be defined as `low` priority callback.
    this.fire( 'checkChild:' + def.name );
}

I am fine with both solutions. One thing that I don't like about the second solution a bit is that we have a method (childCheck()) which behaves much like a decorated method but isn't really, which may be confusing? But this is a tiny nitpick.

@scofalik scofalik added type:improvement This issue reports a possible enhancement of an existing feature. package:engine domain:performance This issue reports a problem or potential improvement regarding editor performance. support:2 An issue reported by a commercially licensed client. labels Feb 12, 2024
@scofalik
Copy link
Contributor Author

Summary:

  1. Schema#addChildCheck() and Schema#addAttributeCheck() have a new optional parameter that allows to register the callback for a specific item or specific attribute.
  2. Earlier, when you registered a custom callback, it was fired for every checkChild() and checkAttribute() call. With multiple features and multiple custom callbacks, it could lead to performance problems (lagging when typing).
  3. The way how it works is very simple:
    1. If you registered addChildCheck( cb, 'foo' ) it will be called when checkChild( ctx, 'foo' ) is called.
      1. Mind, that checkChild( ctx ) also checks the whole ctx. If 'foo' is inside ctx the callback will be fired too.
    2. If you registered addAttributeCheck( cb, 'bold' ) it will be called when checkAttribute( ctx, 'bold' ) is called.
      1. Mind, that you register attribute check for a model attribute not a model item.
  4. Generic callbacks are still available.
    1. Of course, generic callbacks are not recommended as they can lead to performance problems. However, sometimes, they can't be avoided.
  5. checkChild() and checkAttribute() are still decorated so you can add listeners to the events. This is not recommended as it can also lead to performance problems.
  6. The order of performing checks is as follows:
    1. All callbacks added as events listeners with highest and high priority.
    2. All generic callbacks. Among them, order as added in code.
    3. All specific callbacks. Among them, order as added in code.
    4. Declarative checks.

niegowski added a commit that referenced this issue Jul 30, 2024
Feature (engine): `Schema#addChildCheck()` and `Schema#addAttributeCheck()` can now register a callback for a specific item or attribute, which should improve performance when using custom callback checks. Callback checks should be added only for specific item or attribute if possible. See the API reference. Closes #15834.

Fix (engine): `Schema#checkChild()` will now correctly check custom callback checks for each item in the context.

Other (engine): Introduced `Schema#trimLast()`.

Internal: Used specific callback checks in some of the features.

Docs (engine): Improved schema deep dive guide and added missing section related to attributes custom callback checks.

MINOR BREAKING CHANGE: `Schema` callbacks added through `addChildCheck()` will no longer add event listeners with `high` priority and will no longer stop `checkChild` event. Instead, these callbacks are now handled on `normal` priority, as a part of the default `checkChild()` call. This also means that listeners added to `checkChild` event on `high` priority will fire before any callbacks added by `checkChild()`. Earlier they would fire in registration order. This may impact you if you implemented custom schema callback using both `addChildCheck()` and direct listener to `checkChild` event. All above is also true for `addAttributeCheck()` and `checkAttribute` event and callbacks.
@CKEditorBot CKEditorBot added this to the iteration 76 milestone Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:performance This issue reports a problem or potential improvement regarding editor performance. package:engine support:2 An issue reported by a commercially licensed client. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants