[PERF] Don't use array methods #3988

chadhietala · 2015-12-11T22:57:54Z

We shouldn't use array methods as they allocate and close over items that need to be GC'd. Memory preasure can be an issue and we should do everything to reduce that preasure.

stefanpenner · 2015-12-11T23:21:42Z

addon/-private/serializers/embedded-records-mixin.js

-    return Ember.A(hasMany).map((embeddedSnapshot) => {
-      var embeddedJson = embeddedSnapshot.record.serialize({ includeId: true });
+    let manyArray = Ember.A(hasMany);
+    let ret = [];


Let's pre alloc the array, looks like we know the size

stefanpenner · 2015-12-12T00:03:29Z

addon/-private/serializers/embedded-records-mixin.js

@@ -450,11 +456,13 @@ export default Ember.Mixin.create({
  */
  _extractEmbeddedHasMany(store, key, hash, relationshipMeta) {
    let relationshipHash = get(hash, `data.relationships.${key}.data`);
+    let hasMany = new Array(relationshipHash.length);


maybe do this after the conditional on 460

chadhietala · 2015-12-12T04:43:23Z

Windows failed because its windows. I didn't realized that some of the array methods were actually Maps being based around that conforms to the iterable interface.

stefanpenner · 2015-12-12T05:08:55Z

addon/-private/system/store.js

@@ -578,11 +578,13 @@ Store = Service.extend({
  */
  findByIds(modelName, ids) {
    assert('Passing classes to store methods has been removed. Please pass a dasherized string instead of '+ Ember.inspect(modelName), typeof modelName === 'string');
-    var store = this;
+    let promises = [];


topaxi · 2015-12-12T12:53:18Z

Just curious, how much does this affect perf and gc pressure? :)

chadhietala · 2015-12-12T18:33:17Z

TL;DR Don't put things into new generation space if you don't have to.

@topaxi It depends on several factors but the largest is the granularity of your models and relationships between them. On mobile devices where you are much more constrained you need to be very observant of allocations. Contrary to popular belief engines like V8 create a "context" object when you create a closure, regardless of "functional is future" soup of the day 😉. From my understanding*, these are new and short lived they are placed into new generation space which has a size limit. Once that limit is reached a minor GC scavenger runs to release the unreachable. This must stop the world to collect, so if you can perform the same amount of work with less allocations you should.

This is my interpretation of this and it may be incorrect. For more information you might want to read this. https://strongloop.com/strongblog/node-js-performance-garbage-collection/

topaxi · 2015-12-12T18:36:42Z

@chadhietala thanks a lot for the detailed explanation!

fivetanley · 2015-12-13T01:57:02Z

Does this have benefits on runtimes that aren't V8?

fivetanley · 2015-12-13T01:59:16Z

It would also be great to see before/after numbers from this commit.

krisselden · 2015-12-13T01:59:34Z

Why would it not? All of them allocate for closures and not being careful with them is problematic.

krisselden · 2015-12-13T02:08:57Z

The is an excessive amount of looping and nested looping for relationship setup especially if you have not specified the inverse. The forEach and map closures likely could be made to avoid repeatedly creating them but in hot paths we've already demonstrated this is a hot path, profile an app that uses ember data, you will see relationshipsByName.

No, one single spot of waste is going to make a huge difference but we have to get rid of a lot of them to have an big impact. When a low end android phone is about 10x slower it becomes a bigger deal.

stefanpenner · 2015-12-13T02:17:03Z

addon/-private/system/model/errors.js

@@ -272,14 +272,24 @@ export default Ember.ArrayProxy.extend(Ember.Evented, {
    @private
  */
  _findOrCreateMessages(attribute, messages) {
-    var errors = this.errorsFor(attribute);
+    let errors = this.errorsFor(attribute);
+    let _messages = [];


Are errors generated often enough for this to matter?

fivetanley · 2015-12-13T02:19:13Z

Often these kind of commits feel like insider baseball. It'd be great if we could measure these like we do in the Ember Performance Suite. Chad's explanation was great and needed, and having documentation in the form of Pull Requests or commit messages gives us resources to come back to for those of us who aren't familiar with performance tuning.

stefanpenner · 2015-12-13T02:26:42Z

addon/-private/serializers/embedded-records-mixin.js

+    let manyArray = Ember.A(hasMany);
+    let ret = new Array(manyArray.length);
+
+    for (let i = 0, l = manyArray.length; i < l; i++) {


Caching the lengths isn't something I believe we need to do, lets not unless it provides some measurable difference or if the algorithm explicitly calls for it. (The optimizing compiler should be more capable here then us)

I am unsure if assignment/insertion into an array internally requires a bounds check (i'll poke around and see), but I suspect if the loop calls out to a non-inlined function it will still have to, and if it doesn't both bound checks will be optimized out anyways. Which would mean in both cases the length caching provides no value, as the operation in the loop body must already do so, that is unless length itself is some strange foreign non-inlining accessor with evil side-affects. If someone does that, they deserve some special place in hell.

stefanpenner · 2015-12-13T02:45:02Z

@krisselden i suspect ^ will have measurable improvements, and even if there are none (unlikely) displaying the due diligence is important.

Doing so will also help others build a better mental model for this sort of optimization.

runspired · 2015-12-13T03:05:04Z

I'd love to see before/after data, cause to me this looks potentially like we're trading a small amount of execution time and more verbose code for a smaller chance of hitting a blocking GC.

stefanpenner · 2015-12-13T06:32:14Z

@runspired it's a known improvement. I'm merely asking for confirmation of the impact. If we don't see an improvement it is likely the result of some other issue. Which would be worth exploring further as it may uncover something we are not yet sure about.

The explicit code, is brain dead simple for the jit to handle, the callback variant is significantly more complex and as far as we know no runtime can yet reduce the existing code into anything nearly as performant and as the patched variant should be. Maybe someday we will be blessed with more zero cost abstraction run times...

These loops are hit so often, and are themselves in other loops it's worth ensuring we aren't just trolling ourselves with some slip up

chadhietala · 2015-12-14T00:42:27Z

Will try to grab numbers tomorrow.

We shouldn't use array methods as they allocate and close over items that need to be GC'd. Memory preasure can be an issue and we should do everything to reduce that preasure.

chadhietala · 2015-12-14T19:49:57Z

Probably some conflation going on here (due to using live data) but appears to confirm the hypothesis. The selected size is the boot of the app.

Before:

After:

[PERF] Don't use array methods

stefanpenner · 2015-12-16T21:23:55Z

@fivetanley my above comment has not been addressed #3988 (comment)

fivetanley · 2015-12-16T21:49:21Z

We decided to merge this in the core team meeting. The Ember and Ember Data codebases both use this pattern of caching length, it doesn't seem enough to block merging it.

stefanpenner · 2015-12-16T22:22:09Z

@fivetanley can you address it then please, it would be unfortunate to continue to propogate this cargo culting

stefanpenner reviewed Dec 11, 2015
View reviewed changes

chadhietala force-pushed the allocating-alex branch 5 times, most recently from 465ff85 to 76be689 Compare December 11, 2015 23:45

stefanpenner reviewed Dec 12, 2015
View reviewed changes

chadhietala force-pushed the allocating-alex branch 5 times, most recently from 8ea0f04 to 268c252 Compare December 12, 2015 04:34

stefanpenner reviewed Dec 12, 2015
View reviewed changes

chadhietala force-pushed the allocating-alex branch from 268c252 to a308f0b Compare December 12, 2015 18:11

stefanpenner reviewed Dec 13, 2015
View reviewed changes

[PERF] Don't use array methods

0e355c0

We shouldn't use array methods as they allocate and close over items that need to be GC'd. Memory preasure can be an issue and we should do everything to reduce that preasure.

chadhietala force-pushed the allocating-alex branch from a308f0b to 0e355c0 Compare December 14, 2015 19:04

fivetanley added a commit that referenced this pull request Dec 16, 2015

Merge pull request #3988 from chadhietala/allocating-alex

ed6679f

[PERF] Don't use array methods

fivetanley merged commit ed6679f into emberjs:master Dec 16, 2015

topaxi mentioned this pull request Dec 18, 2015

[CLEANUP] Don't cache length in for loops #4015

Merged

bmac mentioned this pull request Dec 23, 2015

use for (var key in obj) IFF #3643

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PERF] Don't use array methods #3988

[PERF] Don't use array methods #3988

chadhietala commented Dec 11, 2015

stefanpenner Dec 11, 2015

chadhietala Dec 11, 2015

stefanpenner Dec 12, 2015

chadhietala commented Dec 12, 2015

stefanpenner Dec 12, 2015

topaxi commented Dec 12, 2015

chadhietala commented Dec 12, 2015

topaxi commented Dec 12, 2015

fivetanley commented Dec 13, 2015

fivetanley commented Dec 13, 2015

krisselden commented Dec 13, 2015

krisselden commented Dec 13, 2015

stefanpenner Dec 13, 2015

fivetanley Dec 13, 2015

fivetanley commented Dec 13, 2015

stefanpenner Dec 13, 2015

stefanpenner commented Dec 13, 2015

runspired commented Dec 13, 2015

stefanpenner commented Dec 13, 2015

chadhietala commented Dec 14, 2015

chadhietala commented Dec 14, 2015

stefanpenner commented Dec 16, 2015

fivetanley commented Dec 16, 2015

stefanpenner commented Dec 16, 2015

[PERF] Don't use array methods #3988

[PERF] Don't use array methods #3988

Conversation

chadhietala commented Dec 11, 2015

stefanpenner Dec 11, 2015

Choose a reason for hiding this comment

chadhietala Dec 11, 2015

Choose a reason for hiding this comment

stefanpenner Dec 12, 2015

Choose a reason for hiding this comment

chadhietala commented Dec 12, 2015

stefanpenner Dec 12, 2015

Choose a reason for hiding this comment

topaxi commented Dec 12, 2015

chadhietala commented Dec 12, 2015

topaxi commented Dec 12, 2015

fivetanley commented Dec 13, 2015

fivetanley commented Dec 13, 2015

krisselden commented Dec 13, 2015

krisselden commented Dec 13, 2015

stefanpenner Dec 13, 2015

Choose a reason for hiding this comment

fivetanley Dec 13, 2015

Choose a reason for hiding this comment

fivetanley commented Dec 13, 2015

stefanpenner Dec 13, 2015

Choose a reason for hiding this comment

stefanpenner commented Dec 13, 2015

runspired commented Dec 13, 2015

stefanpenner commented Dec 13, 2015

chadhietala commented Dec 14, 2015

chadhietala commented Dec 14, 2015

stefanpenner commented Dec 16, 2015

fivetanley commented Dec 16, 2015

stefanpenner commented Dec 16, 2015