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

UI performance #6178

Closed
Reinmar opened this issue Feb 3, 2020 · 4 comments
Closed

UI performance #6178

Reinmar opened this issue Feb 3, 2020 · 4 comments
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:ui resolution:expired This issue was closed due to lack of feedback. status:stale type:improvement This issue reports a possible enhancement of an existing feature. type:performance This issue reports a performance issue or a possible performance improvement.

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 3, 2020

My findings after working on #6175.

ButtonView

We use ButtonView for grid tiles. Unfortunately, this view is very heavy. It has many properties. It renders the tooltip view, keystroke, etc. It binds many things (which creates listeners under the hood). Additionally, instead of just create the instance of it with all the props set at the start (via the constructor), we do change its template and set() on it.

This is the current timeline:

image

This is after I made a simpler GridTileView by catting half of ButtonView:

image

Button interface

Just a random finding – it's there! Unfortunately, it's the only interface that we defined like that. We were hoping to have more classes implementing it but we didn't do this. Instead, we extend the base button class. Which proves that this interface is pointless. Let's remove it.

If we'll ever see a need for a button-like thing, we should then define this buttonish interface. I guess that it will turn out that we rather need a Focusable interface and possibly some other granular/atomic things.

Split UI component interfaces to static and dynamic properties

Right now, every single thing in the ButtonView is dynamic. Meaning that we use things like bind.to() in the template and create some unnecessary subviews in the constructor. This makes this component even heavier than needed and the code more complicated. I guess it's true for some other components too.

Additionally, in reality, some properties are not dynamic anyway. They can only be set before render(). AFAIR this is to allow extending the template after create an instance of that component. We should do our best to avoid this and instead just have dynamic and static properties. Static ones should be passed directly to the constructor so you can use them from the beginning and e.g. avoid creating certain sub views/collections.

The event and observable systems are a bottleneck

We use those systems extremely heavily. They were never optimised nor actually well rethought. There may be quite a lot of potential in working on them. Any improvements will have an effect on the entire editor, not only the UI.

Additionally, we should figure out ways to avoid using this system. E.g.:

  • Avoid having to use bind.to() and other forms of observables. As mentioned above, if we make a maximal number of props static, we will not have to use observables.
  • Many times we add listeners to child component to bubble them up (delegate to one of the parent components). Maybe we need a smarter bubbling mechanism which wouldn't use listeners?
@Reinmar Reinmar added type:improvement This issue reports a possible enhancement of an existing feature. package:ui domain:ui/ux This issue reports a problem related to UI or UX. type:performance This issue reports a performance issue or a possible performance improvement. labels Feb 3, 2020
@Reinmar Reinmar added this to the nice-to-have milestone Feb 3, 2020
@jodator
Copy link
Contributor

jodator commented Feb 3, 2020

We use ButtonView for grid tiles.

It could be dumber than ButtonView for sure - almost static one.

Right now, every single thing in the ButtonView is dynamic.

That looks like the grid tile shouldn't use ButtonView - the problem with it is that is too generic, as you described. For "big" grids it turned out to do too much.

The event and observable systems are a bottleneck

I had similar findings recently when inspecting how the new style processing engine would work. As a first "optimization" I've ditched the previous idea for event base system for simpler (faster) Map lookup.

It would be interesting to re-write this part of code but it might be hard as we have many requirements for observables.

@oleq
Copy link
Member

oleq commented Feb 3, 2020

Right now, every single thing in the ButtonView is dynamic. Meaning that we use things like bind.to() in the template and create some unnecessary subviews in the constructor. This makes this component even heavier than needed and the code more complicated. I guess it's true for some other components too.

Yeah, we recently started moving some observable props to options passed via constructor in some components (ToolbarView?). I agree lots of observable props are useless and were made observable "just in case" and followed this because it became a pattern. We can review that and create fewer listeners in many components.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 10, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:ui/ux This issue reports a problem related to UI or UX. package:ui resolution:expired This issue was closed due to lack of feedback. status:stale type:improvement This issue reports a possible enhancement of an existing feature. type:performance This issue reports a performance issue or a possible performance improvement.
Projects
None yet
Development

No branches or pull requests

5 participants