Opened 5 years ago

Closed 4 years ago

#10903 closed Bug (fixed)

Performance improvements for add|remove|hasClass methods

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: General Version:
Keywords: Cc: joel.peltonen@…

Description

While profiling CKEditor initialization I notice that:

  • removeClass takes >6.5% of total CPU usage,
  • addClass takes >3.5% of total CPU usage.

It is possible to use native API to manage classes: https://developer.mozilla.org/en-US/docs/Web/API/element.classList

Change History (10)

comment:1 Changed 5 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: newassigned

comment:2 Changed 5 years ago by Piotrek Koszuliński

I pushed t/10903. I'm not certain though, what's the improvement. With opened Chrome Dev Tools I see 10x better results, but with closed there's no difference. There may be two reasons:

  • JIT compiler optimizing hot code when running those micro benchmarks with closed tools (but it would be strange if JIT wasn't running with opened dev tools).
  • CKPerf being inaccurate. I wrote it for long, asynchronous tests like editor initialization, set data, etc. It does not work well with micro benchmarks. I'll try another tool on improving CKPerf.

comment:3 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.0

It will be better to make this change in a major release than a minor one, even though it is a matter of internal implementation, because some may use addClass/removeClass incorrectly with more than one class. After change it will throw an error.

comment:4 Changed 4 years ago by Joel

Cc: joel.peltonen@… added

How about testing with a cumulative test specifically designed for this? For example running hasClass 10000 times in a loop and measuring the time taken by the execution? Similarly to what jsperf might do. I use these a lot so this is a nice improvement. Add cc.

comment:5 Changed 4 years ago by Piotrek Koszuliński

See comment:2. I did performance tests, but couldn't get stable results out of them. This code is too insignificant to test it - just like every microbenchmark it's unreliable. But of course we want to make this change, because it's definitely promising.

comment:6 Changed 4 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed branch:t/10903b. I also refactored the code a little bit and fixed other incorrect addClass usage. The change brings a 0-2% performance improvement depending on browser and what's tested. Although, the results were so unstable, that I wouldn't take them seriously. Still, I like this change, because 200 a/r/hClass calls we make when initialising a single editor is a lot and switching to modern, native API like classList seems to be a reasonable decision.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:7 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

Because addClass with multiple parameters was used in multiple places in the core we should consider backward compatibility. Despite the fact that multiple classes in addClass parameter is not described in the documentation, some developers may found usage of it and created similar code. We can just split the addClass parameter and call the new method for each. It should not be complex nor slow, considering the fact that addClass parameters are not long.

Also, as long as there are pretty good tests for addClass and removeClass, there should be tests for hasClass as well. Of course hasClass is covered by many other tests, but if this method contains some errors it should be clean that hasClass, not any other function, is the reason of such error.

comment:8 Changed 4 years ago by Piotrek Koszuliński

I was hesitating, but after even more thinking I want to keep this simple:

  • This only worked because of the specific way how classes were added (by appending them to className). If someone based his code on that, then it's his/her fault.
  • If we start handling a multi-class string, we should also think about starting handling an array...
  • ... which should also be handled in removeClass() to assure consistency; I don't want to explain now that only our addClass() handles multiple classes, because someone used it this way while, by coincidence, it worked.
  • All this increases complexity and code size.
  • There are veeeery few cases in which we add more than one class at a time.

comment:9 Changed 4 years ago by Piotrek Koszuliński

Status: review_failedreview

Back on review after rebasing and adding tests for hasClass(). Thanks for pointing that it didn't have tests, because there's an odd thing that it's case sensitive, while addClass() is not. That's how it was before, so I'm not changing this because it also doesn't make any difference, because everyone should use them as case sensitive methods. Still - good to have tests that the behaviour didn't change.

comment:10 Changed 4 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

Agreed. Using not documented method feature is like using private methods, it can change.

Closed: git:0836608

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy