chore(benchpress): add a ngClass benchmark #15243
Conversation
drpicox
changed the title
The cases that should benefit most are:
1. When using large objects as values (e.g.: `{loaded: $ctrl.data}`).
2. When using objects/arrays and there are frequent changes.
3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s).
The differences in operations per digest include:
1. `Regular expression (when not changed)`
**Before:** `equals()`
**After:** `toClassString()`
2. `Regular expression (when changed)`
**Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()`
**After:** 2 x `split()`
3. `One-time expression (when not changed)`
**Before:** `equals()`
**After:** `toFlatValue()` + `equals()`*
4. `One-time expression (when changed)`
**Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()`
**After:** `copy()`* + `toClassString()`* + 2 x `split()`
5. `$index modulo changed`
**Before:** `arrayClasses()`
**After:** -
(*): on flatter structure
In large based on angular#14404. Kudos to @drpicox for the initial idea and a big part
of the implementation.
Closes angular#14404
drpicox
force-pushed
the
bp-ngClass
branch
2 times, most recently
from
3969f4a to
d095a0c
Compare
There was a problem hiding this comment.
Nice! It is obviously not exhaustive (e.g. no array or one-time bindings or alternating $indexes), but it covers most common usecases (and is of course much better than what we (didn't) have before) 👍
So, if I understand correctly, #15228 is indeed a major improvements in most cases (but a slight de-opt in some). I think we can live with that, given the benefits.
💯 on putting this together and doing the comparisons!
| Implementation:<br> | ||
| <label> | ||
| <input type="radio" ng-model="benchmark.implementation" value="tableOptimized"> | ||
| Table optimized <code>ng-class="'success' && todo.completed"</code> |
There was a problem hiding this comment.
This seems strange. What is it supposed to do?
There was a problem hiding this comment.
You probably meant todo.completed && 'success' 😃
There was a problem hiding this comment.
Yes, indeed, I was too quick commiting 👼
| </label><br> | ||
| <label> | ||
| <input type="radio" ng-model="benchmark.implementation" value="list"> | ||
| Table <code>ng-class="{completed: todo.completed, urgent: todo.urgent, important: todo.important"}</code> |
| this.importantPeriodicity = 13; | ||
| this.urgentPeriodicity = 29; | ||
| this.todos = buildTodos(100); | ||
| configureTodos(this.todos, 0, this); |
There was a problem hiding this comment.
Are these two lines necessary (since we have the setup step)?
There was a problem hiding this comment.
They are helpful.
These lines helped me to make sure that the benchmark does what it means to do, and they are also helpful because now it shows a small preview of the selection that you have done: list, table, single value.
And I do not rebuild the list of 100 todos at the end of the tests to induce a refresh: the list/table is empty and it is restored doing refresh (which is helpful to clear memory).
|
I like the result, I usually write guides or check the code in critical zones making sure that my colleagues are using optimized ngClass values. With #15228 seems that it will work well from time 0. I have rewritted the code a little bit, and fixed the "spelling" problems. It is not exhaustive because my resources are limited. I have just added few cases that I know that are significative. I expect that with time and other PRs more cases will be added. One question:
It would be nice to have benchmarks running automatically and follow the chart of performance changes through time. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...): benchmark
Adds a benchmark to measure the impact of changes in ngClass.
What is the current behavior? (You can also link to an open issue here): #15228 (comment)
What is the new behavior (if this is a feature change)? no change
Does this PR introduce a breaking change? no
Please check if the PR fulfills these requirements
Tests for the changes have been added (for bug fixes / features)Docs have been added / updated (for bug fixes / features)Other information:
It adds a benchmark in
benchmarks/ng-class-bpThe benchmark was designed to measure the changes introduced by: #15228
It has the following steps:
There are three versions to paint:
ng-class="'success' && todo.completed"ng-class="{success: todo.completed}"ng-class="{completed: todo.completed, important: todo.important, urgent: todo.urgent}"ng-class="{success: !!$ctrl.todos, danger: !$ctrl.todos}"ng-class="{success: $ctrl.todos, danger: !$ctrl.todos}"I ran them over the previous version of ngClass and the version of #15228 , the results are:
I also modified #15228 to use compile instead of link (commented in #15228 (comment) ). Results are:
So, as a conclusion, it seems that the #15228 optimizes automatically code in the same fashion that an experienced angular programmers would do manually.
In the other hand, given that 1 frame is around 16ms, we should give 8ms to the browser to update, it leaves 8ms for the application to work. Experiments show that digest loop times moves from values like 8 or 10ms to just 1ms in large lists, or 0.01ms in a single big object condition.