make isEqual() decide equality of property values. #10166
Conversation
For vanilla properties, calling `set` with the current value, will not
fire any property changes. Currently, this comparison of "sameness"
happens with the `===` operator which compares object identity.
This change uses the `isEqual` method to compare property values
instead. In most cases, this exactly the same as `===`, but does allow
for programmer-defined objects to decide whether they are equal to each
other and whether notifications should be fired when one replaces the
other.
To define a simple, immutable value object today that works seemlessly
with the Ember change notification system today, you have to make an
identity map coupled with a factory method (which could be an overridden
version of `create`) such that:
Car.for("ford", "mustang") === Car.for("ford", "mustang") //=> same object
Until such time as `WeakReference` is a thing in javascript, this
approach becomes unwieldy for types that have a large or perhaps infinite
number of values, since ensuring referential integrity means retaining
every instance ever created.
By allowing types to define their own notion of equality as pertains to
property access, you allow programmers to enrich the vocabulary of basic
property values without having to worry about over-chattiness, or worse
infinite-loopiness of observer notification.
|
It seems good, but it is in a very concerning area of the codebase. This could massively tank performance if not carefully considered and tested. |
|
Maybe an addon based on https://www.npmjs.com/package/ember-computed-change-gate would be more appropriate for providing isEqual() based equality checking where desired so mainline performance is not impacted. |
|
@rwjblue agree 100% The total running time of the test suite was not affected, but a more comprehensive benchmark should be taken into consideration. I couldn't figure out how to run the benchmarks in the |
|
@cowboyd - We have recently started using https://github.com/eviltrout/ember-performance for performance profiling. Might grab a copy of that and report some before/after numbers of the various benchmarks. |
|
@cowboyd ember-performance is pretty easy to use, take a look. There are micro and macro benchmarks and you will obviously want the former. You should try timing a variety of types. |
|
Thanks for the tips. I ran the suite twice: both against the current canary build and with this change applied. These four runs were done on Chrome only (is there a policy on which browsers we want to do performance checks on?) And here are the results: Note that the Before1.2.After1.2.I don't see anything that screams disaster here, or even indicates a real degradation, but we should definitely explore more. What do you think would be a sufficient sample set? <conjecture> In practice, this change might actually save on net computation since it is more restrictive about when change notifications are dispatched, and it is the notifications, not the change itself that is expensive. </conjecture> |
|
I've composed a summary table of the object accessors and initialization rows to make comparison more obvious. There is still quite a delta across the board with those benchmarks, in some cases its an improvement and others not, so I'm not sure how reliable the figures are in actuality. The baseline object initialization has a large delta. What is the difference in benchmark sets 1 and 2?
|
|
@ahacking That is much easier to read, thanks. To answer your question, there is no difference between 1 and 2, they are both running canary before and canary + isEqual patch after, so four times, one right after another, before and after, before and after. As you can see, there are some significant deltas between the runs on unpatched Canary without any changes, so I don't think we can conclude anything other than there aren't any gross degradations that we wouldn't expected to see as normal variation upon subsequent runs of the benchmarks against the unchanged codebase. However, I don't think that serves as proof enough. The question is, what would? |
|
I think the test needs be be repeatable so that variances between runs are reduced as much as possible. This may mean restarting the browser from a clean slate each time. Then you have OS variances due to memory, swap, I/O and whatever random activity on the local LAN happens to be consuming IO interrupts and stealing away CPU to worry about as well. Maybe the OS and other variances can be discounted if some raw JS benchmarks are run and it results in low skew between runs. Kind acts as a test run self check. That's why when I saw the baseline having so much variance I couldn't really treat the results as meaningful. There is a good writeup on benchmark testing on JS by the jsPerf author. Maybe Ember needs to adopt the use of benchmark.js ?? I think it would be really useful to have solid perf tests so performance gains and regressions can be easily and reliably determined. |
|
@ahacking We are extremely interested in contributors wanting to work on a benchmark suite. benchmark.js is great for microbenchmarks. |
|
I agree @ahacking. There are two issues I see, the first is that there is too much variance in the mean between runs agains the same codebase (the two before and after runs in my dataset were taken about twenty minutes apart). It appears that, as you pointed out, there is a lot of context that can change significantly with time. The second, is that there is no way to compare benchmarks between different ember versions without running two suites and then eyeballing, and even then its hard to be sure about the results since its hard to determine in what way the computational context has changed in between runs. Assuming that the computational context changes very little over very short periods of time, one approach might be to interleave the sampling from the two different Ember versions, so that the same number of samples are subjected to the same temporal effects. In other words, for each benchmark, take twenty samples against version A, twenty against version B, another twenty against version A, another twenty against version B. If the initial assumption holds, and there is no performance regression, then we would expect the same mean (or one verifiable with a p-value analysis). I don't know how feasible this is, and Statistics aren't my long suit, but having an automated process to compare to versions that would reliably spot performance differences would be nice to have. |
|
I've been thinking if it would be possible to load each ember/benchmark in separate iframes and then test each version of the framework with benchmark.js and using oasis.js for messaging each iframe instance. That would allow benchmark.js to use its approach for removing variance and still keep each ember version isolated. |
|
this actually seems like a pretty hefty change, we should think about it more closely. |
|
@stefanpenner - agreed. In theory we could make |
|
@stefanpenner @rwjblue should we close this? |
|
@Serabe I believe so. |
For vanilla properties, calling
setwith the current value, will notfire any property changes. Currently, this comparison of "sameness"
happens with the
===operator which compares object identity.This change uses the
isEqualmethod to compare property valuesinstead. In most cases, this exactly the same as
===, but does allowfor programmer-defined objects to decide whether they are equal to each
other and whether notifications should be fired when one replaces the
other.
To define a simple, immutable value object today that works seemlessly
with the Ember change notification system today, you have to make an
identity map coupled with a factory method (which could be an overridden
version of
create) such that:Car.for("ford", "mustang") === Car.for("ford", "mustang") //=> same object
Until such time as
WeakReferenceis a thing in javascript, thisapproach becomes unwieldy for types that have a large or perhaps infinite
number of values, since ensuring referential integrity means retaining
every instance ever created.
By allowing types to define their own notion of equality as pertains to
property access, you allow programmers to enrich the vocabulary of basic
property values without having to worry about over-chattiness, or worse
infinite-loopiness of observer notification.