es6 RecordArrayManager - #4708
Conversation
|
This may be ok here. Although it is unclear if others don't reopen/extend this class. If that isn't the case, and the output isn't unreasonably large. This may be fine. @runspired / @krisselden and myself should powow soon to fixup the cost centers insides of creation (of first object when mixins need application) in ember's core_object. I believe there is a relatively clear path to improving that, that would just have mega leverage across the whole ecosystem. I also believe this can be used to help bridge the ES6 <=> CoreObject worlds. As they really are not very dissimilar. |
|
@stefanpenner this isn't something others can extend, but it is something that could theoretically have been reached into, found, and reopened. Is there a list of ember-data addons that we generally search for private usage in? cc @bmac |
| let modelName = params.modelName; | ||
| delete params.modelName; | ||
|
|
||
| console.time('ember-data'); |
There was a problem hiding this comment.
^ should we leave this?
| // and clutter our benchmarks and make it harder to time. | ||
| records.toArray(); | ||
| heimdall.stop(token); | ||
| console.timeEnd('ember-data'); |
There was a problem hiding this comment.
^ should we leave this?
There was a problem hiding this comment.
I've found it invaluable and it's in our dummy app so it should be ok.
There was a problem hiding this comment.
however I also just want to bake this into heimdall.
| } | ||
|
|
||
| get _recordArrays() { | ||
| return this.__recordArrays; |
There was a problem hiding this comment.
who sets __recordArrays ?
There was a problem hiding this comment.
perhaps no one at the moment, but our tests often introspect state so moving it to a similar getter will let us test correctly
was this one sample? |
|
@stefanpenner didn't you reopen this at Yapp for the subscribe stuff? |
|
/cc @lukemelia |
Unclear, I do also share your concern this is risky relative to the value. (making mixin stuff faster, would have nice leverage...) |
|
Also, the proto() is only doing work here because of lookupFactory extend. |
|
As far as wins go, this one is pretty small, but one of hundreds of paper cuts we die from when out together. I don't actually think the win here is in this small perf gain as much as it is in building a core set of primitives that don't depend on Ember unless they truly need to. If Ember.Object and lookupFactory do have their costs lowered, I'm still unconvinced that this would belong remaining one. |
|
I just checked out the 47 Ember Data Adapter addons from ember observer into a directory and grepped for I've updated my search to include the https://emberobserver.com/categories/ember-data-extensions and still found nothing that references |
|
@runspired This great! Is there any way to get some more exhaustive benchmarks? |
|
@chadhietala the benchmark runs for repeated iterations are roughly similar. Since we strip the classCallCheck I'd be surprised if the compiled output were larger vs equal or smaller than the compiled output for the Ember.Object version. |
|
Would heavily prefer this to be feature flagged. |
|
@fivetanley What is your thinking on why the feature flag for this change. In theory it should be invisible to users and doesn't provide and new functionality? |
|
@bmac @fivetanley I think this is the sort of thing where if we send it out, and it breaks people's apps, its small and we just revert it. Don't think it warrants a flag. |
|
cc @bmac @fivetanley for review and suggested next steps. |
|
@runspired this just needs a rebase and should be good to merge. |
|
@bmac rebased :) |
6 participants
There wasn't really a good reason for this singleton being an Ember.Object (it's not resolved / not in the container / no ember-y things in use), and converting it shaves 33% off it's instantiation time in addition to giving us nice es6 syntax (1.53ms => .97ms).
I additionally moved the instantiation of the
OrderedSetused to track theRecordArrays for aninternalModelinto internal models directly instead of having the manager reach in. I'm slightly conflicted on this because of the resulting double-underscore for the cached property, but I prefer to leave objects in control of their own properties.Before
After
