Benchmark tweaks #619
Benchmark tweaks #619
Conversation
| if (diffFunctions.length === 0) { | ||
| diffFunctions.push(auto); | ||
| const newProperty = this._bindFunctionProperty(properties[propertyName], bind); | ||
| if (registeredDiffPropertyNames.indexOf(propertyName) !== -1) { |
There was a problem hiding this comment.
I don't know if it is any more performant, but you could use @dojo/shim/array.includes() here which offloads to native...
There was a problem hiding this comment.
I "think" that indexOf performs better generally across our supported platforms, although native includes is slightly better on chrome and firefox according to this benchmark: https://jsperf.com/array-indexof-vs-includes
| const diffFunctions = this.getDecorator(`diffProperty:${propertyName}`); | ||
| for (let i = 0; i < diffFunctions.length; i++) { | ||
| const result = diffFunctions[i](previousProperty, newProperty); | ||
| if (result.changed && changedPropertyKeys.indexOf(propertyName) === -1) { |
|
|
||
| this._properties = diffPropertyResults; | ||
|
|
||
| if (changedPropertyKeys.length) { | ||
| if (changedPropertyKeys.length > 0) { |
| } | ||
|
|
||
| if (runReactions) { | ||
| this._mapDiffPropertyReactions(properties, changedPropertyKeys).forEach((args, reaction) => { |
There was a problem hiding this comment.
speculating, but with us targeting the down emit and access this in the arrow function, it might be an opportunity to create a bound callback that we cache if this gets called a lot?
There was a problem hiding this comment.
It's not exactly a hot path as only gets called if there is a registered diffProperty.
| } | ||
|
|
||
| if (dNode.properties.bind === undefined) { | ||
| (<any> dNode).properties.bind = this; |
There was a problem hiding this comment.
why if dNode.properties.bind didn't require a cast on dNode, it does now?
There was a problem hiding this comment.
Because it is readonly...
agubler
force-pushed
the
benchmark-tweaks
branch
from
975ae0b
to
a25e175
Compare
|
@kitsonk any more feedback / comments on this one? |
| this._bindFunctionPropertyMap = new WeakMap<(...args: any[]) => any, { boundFunc: (...args: any[]) => any, scope: any }>(); | ||
| this._registries = new RegistryHandler(); | ||
| this._registries.add(registry); | ||
| this.own(this._registries); | ||
| this._boundRenderFunc = this.render.bind(this); |
There was a problem hiding this comment.
More of general observation of why we don't use initializers generally... like:
class WidgetBase {
private _properties = {} as P;
private _registries = new RegistryHandler();
}It would cut out a fair amount of typing and is even more readable IMO.
There was a problem hiding this comment.
I prefer initialising inline also, seemed unrelated to these changes though.
| if (diffFunctions.length === 0) { | ||
| diffFunctions.push(auto); | ||
| const newProperty = this._bindFunctionProperty(properties[propertyName], bind); | ||
| if (registeredDiffPropertyNames.indexOf(propertyName) !== -1) { |
| @@ -198,13 +191,14 @@ export class WidgetBase<P = WidgetProperties, C extends DNode = DNode> extends E | |||
| this._properties = <P> {}; | |||
There was a problem hiding this comment.
Would Object.create(null) perform better? I don't if this gets overwritten or not, but if it doesn't, I suspect it will be a minor performance improvement in property lookups.
There was a problem hiding this comment.
I think generally it does, I don't think it'll reflect in the benchmarks but I'll update it regardless.
There was a problem hiding this comment.
Actually hasOwnProperty is used by Maquette on properties so we need it to be an object.
There was a problem hiding this comment.
DOH! Sorry, I think I remember that now. 😢 Is it worth it to PR maquette to use Object.hasOwnProperty.call(props, key) so that Object.create(null) can be used, or is it just a minimal improvement thing that it wouldn't matter?
There was a problem hiding this comment.
The honest answer is I don't know how significant it would be, I do suspect minimal.
agubler
force-pushed
the
benchmark-tweaks
branch
2 times, most recently
from
83fd64e
to
4a8426d
Compare
agubler
force-pushed
the
benchmark-tweaks
branch
from
4a8426d
to
743df03
Compare
Type: feature
The following has been addressed in the PR:
Description:
Some performance tweaks based on the
js-framework-benchmarkresults.Before and after (tweak) results running on my mac with 20 iterations.