Performance improvements for add|remove|hasClass methods

[Piotrek Koszuliński]

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

[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.

[Piotrek Koszuliński]

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.

[Joel]

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.

[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.

[Piotrek Koszuliński]

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.

[Piotr Jasiun]

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.

[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.

[Piotrek Koszuliński]

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.

[Piotr Jasiun]

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

Closed: ​git:0836608