#4103 check if obj is Model in Collection.get #4104

Merged
merged 1 commit into from Dec 1, 2016

Projects

None yet

3 participants

@ksladkov
Contributor

No description provided.

@akre54
Collaborator
akre54 commented Nov 29, 2016

Do you have a quickie jsperf to show that the performance hit for this isn't too terrible?

@akre54
Collaborator
akre54 commented Dec 1, 2016

I'm getting a script error on that jsperf page. Anything obvious?

@akre54
Collaborator
akre54 commented Dec 1, 2016

I ran it quickly in the console (which doesn't have the scientific rigor of jsperf), but these are the numbers I got:

console.time(); _.times(200, i => collection1.get(json)); console.timeEnd(); // => 0.249ms
console.time(); _.times(200, i => collection2.get(json)); console.timeEnd(); // => 0.301ms

console.time(); _.times(200, i => collection1.get(model)); console.timeEnd(); // => 0.528ms
console.time(); _.times(200, i => collection2.get(model)); console.timeEnd(); // => 1.31ms
@ksladkov
Contributor
ksladkov commented Dec 1, 2016

It's surprising for me. I just checked this jsperf on my tablet and got this screen
When I ran this on laptop, I had got 3-4 times more performance, and the ratio was quite similar: collection1.get() was ~5% more performant, but no more.

@akre54
Collaborator
akre54 commented Dec 1, 2016

Hm. Ok, getting rid of the duck-typing is reason enough for me. Merging.

@akre54 akre54 merged commit 7da4ed7 into jashkenas:master Dec 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akre54 akre54 added the change label Dec 1, 2016
@matthijsgroen

I ran into this issue today, when implementing http://jsonapi.org/
glad this is fixed already. Is there a release scheduled ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment