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

Added mentions feed throttling #1983

Merged
merged 3 commits into from
Jun 12, 2018
Merged

Added mentions feed throttling #1983

merged 3 commits into from
Jun 12, 2018

Conversation

jacekbogdanski
Copy link
Member

@jacekbogdanski jacekbogdanski commented May 17, 2018

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

Tests are available for autocomplete throttling.

What changes did you make?

I added data request throttling for each feed type with default throttling time 200 ms
I did some testing on this issue and I think that proposed throttling thresholds in #1972 ticket (70-120ms) is too low for performance profit.

Also I tried to use CKEDITOR.tools.eventsBuffer for throttling implementation but this function doesn't allow to cancel buffered callback. It's always fired after provided timeout.

Closes #1972

@mlewand mlewand self-requested a review May 18, 2018 11:44
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Also I tried to use CKEDITOR.tools.eventsBuffer for throttling implementation but this function doesn't allow to cancel buffered callback. It's always fired after provided timeout.

In this case a much, much simpler approach is to provide a new feature to disable the eventBuffer instance.

We could go either with eventBuffer.disable() or eventBuffer.destroy(). The later is more consistent with our general code base.

This feature should be extracted to a separate issue (deserves API changelog) , that should be treated as an upstreamfor this one.

@jacekbogdanski
Copy link
Member Author

jacekbogdanski commented May 20, 2018

I find out that there is eventsBuffer.reset function which allows you to clear scheduled timeout, so there is no need for additional function. Although I had to add an additional flag to eventsBuffer.input to postpone buffer execution.

There is one very important issue with eventsBuffer and I created separate t/1972-dirty branch for my changes because I think that eventsBuffer is not a good option for this implementation.

eventsBuffer allows you to pass parameters into callback only by binding this object into scopeObj parameter. The problem with this method is that in some cases it will produce a race condition between different callbacks. Slower callback which should be used will get an invalid query replaced by older one. You can test this issue with test URL has no race condition unit test (tests/plugins/mentions).

Of course, I could implement additional logic into eventsBuffer to handle this case but I think it will create much more complicated code than proposed in this PR.

Also, I think that eventsBuffer solves a different problem than we are fighting with. As I understand:

  • eventsBuffer - allows you to execute a function during the given time once. So if we set our eventsBuffer to 200ms and type @, @a, @an separated by 10ms pauses during this 200ms a given callback will be fired once after exactly 200ms (not 220ms). Next, if we type @ann it will start new 200ms time interval. For this 4 characters, we will get 400ms summary throttling time.

  • throttle - required to postpone data request by the given time after each typed character. Using the previous example it will execute the given callback by 10 * 2 + 200 + 200 = 420ms. If you type let's say @ in 10ms and stop at @a the summary time will be 210ms.

In the result data request will be executed much more often when using eventsBuffer and I think it will create some misunderstanding how this feature works (for end-users) because it will require some calculations to predict requests per throttle level.

To get the second results using eventsBuffer I have to use it in invalid way (IMO) by resetting previous callback before every input call. It brings us down to exactly the same code proposed with this PR. Also there is still race condition issue and currently I don't have an idea how to solve this bug (except changing eventsBuffer again).

TBO I think I will have to overcomplicate this issue with eventsBuffer, where currently proposed code (except some simple function extraction) takes a couple of code lines to implement it:

https://github.com/ckeditor/ckeditor-dev/blob/54d9883cf1977b52b44fe690a760cacd586b7c29/plugins/mentions/plugin.js#L197-L208

against:
https://github.com/ckeditor/ckeditor-dev/blob/0c3fcc9bd3da76030f27cd46cd3c8a6459d6eb5b/plugins/mentions/plugin.js#L181-L203

Of course, I could be wrong and my implementation with eventsBuffer may be incorrect. I would really appreciate your opinion @mlewand if you see a better way to utilize eventsBuffer feature in this context.

@mlewand
Copy link
Contributor

mlewand commented May 21, 2018

You're right about the fact that we can't reuse eventsBuffer function to implement this cleanly. We need a throttling function that can accept, and pass arguments in the throttled function.

Also the way I see it I started to think that the throttling should actually be implemented in autocomplete plugin itself (or even on textwatcher level 🤔).

We must not implement throttling logic directly in mentions/autocomplete or related plugin as this is a logic that should be reusable.

So as a solution let's make a CKEDITOR.tools.throttle function, that will return an object with the same API as the one returned by eventsBuffer. Also if there will be a need to, also implement disable function that we were talking about before. This throttle function should be used in autocomplete plugin.

Note that the function should work the same way as eventBuffer does in terms of being called, so, say you have a throttling set to 100 ms and you have following calls:

call no. time offset (relative to first call)
1 0ms
2 20ms
3 40ms
4 120ms

The throttled function should be executed thee times with following offsets:

time offset
0ms
100ms
200ms

Throttling function should be developed as a separate issue (it deserves a separate API changelog entry), so this issue will be dependent on it. Please create the GH issue accordingly and prepare a new PR.

@mlewand
Copy link
Contributor

mlewand commented Jun 8, 2018

This PR needs to be rebased.

@jacekbogdanski
Copy link
Member Author

I think there is no need for copy&paste unit and manual tests from autocomplete throttling. I'm just passing config property into autocomplete, so there is no logic inside mentions plugin.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

LGTM

@mlewand mlewand merged commit 410673e into t/1703-b Jun 12, 2018
@mlewand mlewand deleted the t/1972 branch June 12, 2018 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants