applyTransforms hasOwnProp is mega slow

[stefanpenner]

data/packages/ember-data/lib/serializers/json_serializer.js

Line 112 in e1ca37a

if (!data.hasOwnProperty(key)) { return; }

we should try and merely have sensible behaviour for the if the key is undefined and avoid this costly step until browsers make it fast.

[sly7-7]

@stefanpenner This is the same as #2213

I think @fivetanley said that partial normalisation will not be supported out of the box anymore, in this case, we can simply remove all those checks.

cc/ @wycats @igorT

[fivetanley]

@sly7-7 I think this check is more for stuff like toString etc

[stefanpenner]

@fivetanley doubtful eachAttribute would have to yield something invalid

[bmac]

@stefanpenner would checking to see if (data[key] === undefined) { result in a performance improvement over !data.hasOwnProperty(key)?

[stefanpenner]

@bmac yes

[balinterdi]

I took a stab at fixing this with #4281.

The suggested fix to use data[key] === undefined instead of !data.hasOwnProperty(key) did not work because there are a couple of tests that assert that if the key is present but has a value of undefined or null, it gets serialized to null in both cases.

So I ended up using if (!(key in data)) which seems to work. The caveat is that I'm not sure whether key in data is any faster than data.hasOwnProperty(key). If somebody knows this for sure, it'd be great if they could confirm/refute this, otherwise I'll try to profile it with some app code.

[balinterdi]

I discussed this with @stefanpenner and @bmac at EmberConf and Stef suggested that, since the current fix might actually not improve performance, I look at the tests that fail when I applied the simple property lookup solution (suggested above by Brendan).

More precisely, we don't need to test if undefined is ever deserialized (transformed) to anything since undefined is not a valid value in a server response (in JSON). So summing up, the correct solution is probably to remove those tests and just write if (data[key] === undefined) { return }

[balinterdi]

@stefanpenner @bmac How should I go about this? Should this issue be reopened and then another PR submitted with the solution suggested in my previous comment?

[bmac]

@balinterdi feel free to open another PR with the solution from your previous comment.

[balinterdi]

@stefanpenner @bmac I started removing the test cases that assert that setting a DS.attr to undefined results in the attribute's value to be null (for string and number attrs) or false (for Boolean) values but it occurred to me that attributes cannot only be set from server responses but also directly, in user code (e.g user.set('profession', undefined))).

The tests even account for this as they have both assert.converts and assert.convertsFromServer assertions.

I can't think of a good reason to allow ED users to set their attribute values to undefined when they can set it to null to indicate a missing value but I want to hear your take on this before I embark on this.

If you agree with this, the tests that should be removed are the ones where the attr is set to undefined, be it from the server or directly. Tests where the attr is set to null (both directly and from the server, as null is a valid JSON value) should remain intact.

[stefanpenner]

if they explicit set to undefined, we could detect and go down the sentinel internal value path i talked about. (im not totally sure that would work in this case, but maybe worth exploring)

[bmac]

I believe the relevant code that we care about in this issue https://github.com/balinterdi/data/blob/c2839ac545b25c22c8dc51387761874d9ecdc781/addon/serializers/json.js#L196 only runs when "deserializing" data provided by the server. Ember Data takes another code path when it serializes user provided data (e.g. user.set('profession', undefined)) https://github.com/balinterdi/data/blob/c2839ac545b25c22c8dc51387761874d9ecdc781/addon/serializers/json.js#L1082-L1106

[stefanpenner]

some good news: https://chromium.googlesource.com/v8/v8.git/+/bc8f9a78f05c7a9dce0a112835d797d8082749eb (so the future may be faster, but we should still do this).

[balinterdi]

@bmac I checked and applyTransforms is only called from the normalize method of the json and the json-api serializers. Is that the same thing as you suggest?

@stefanpenner I don't see the value in allowing users to set attribute values (of type string, number and boolean) to undefined to end up with a null value but I guess that this has worked before so regressing on this can potentially break some apps out there that use it. So I'll look into the sentinel value setting.

[bmac]

Yes, normalize is used when reading data in from the backend. serialize is used when sending data out to the backend.

[balinterdi]

Thought about this some more. The test cases that now fail correspond to the use case where store.push(store.normalize('person', data)) is called where person has an attribute that is explicitly set to undefined in data. We agreed this is a non-issue as undefined is an invalid value in json and data is supposed to be the serialized data from the server.

So I think it's fair to remove these test cases as they assert unrealistic situations and just go with if (data[key] === undefined) which is blazing fast.

If you don't object, I'll create the corresponding PR.

[bmac]

Sounds good to me @balinterdi.

[balinterdi]

Pull request submitted at #4311