Closed fabienjuif closed 6 years ago
One selector is a breaking change, this is getState
because it retrieves state
as it is (without any transformation), and before it was:
{
data: {
'1': { some: 'value', id: '1' },
},
keys: ['1'],
array: [
{ some: 'value', id: '1' },
],
initialized: true,
}
Now this is:
{
data: [
[1, { some: 'value', id: '1' }],
]
initialized: true,
}
So if someone use this selectors instead of high level one, this gona break
Perf are ... shitty
// BEFORE (master)
results (ms): {
"writes": {
"total": 3993,
"init": 3,
"set": 59,
"add": 1027,
"update": 1186,
"remove": 1030,
"addOrUpdate": 688
},
"reads": {
"total": 22610,
"getState": 14,
"getKeys": 15,
"getAsArray": 20,
"getLength": 17,
"isInitialized": 11,
"get": 18,
"getByIds": 22437,
"getById": 40,
"hasKey": 38
},
"all": 26603
}
// RERUN_SELECTORS / 100
results (ms): {
"writes": {
"total": 1363,
"init": 3,
"set": 33,
"add": 342,
"update": 371,
"remove": 273,
"addOrUpdate": 341
},
"reads": {
"total": 12034,
"getState": 1,
"getKeys": 2583,
"getAsArray": 2471,
"getLength": 0,
"isInitialized": 0,
"get": 2323,
"getByIds": 1589,
"getById": 1535,
"hasKey": 1531
},
"all": 13397
}
// RERUN_SELECTORS / 100 with memoization
results (ms): {
"writes": {
"total": 1370,
"init": 3,
"set": 108,
"add": 270,
"update": 374,
"remove": 266,
"addOrUpdate": 349
},
"reads": {
"total": 6925,
"getState": 1,
"getKeys": 3,
"getAsArray": 3,
"getLength": 1,
"isInitialized": 0,
"get": 2291,
"getByIds": 1571,
"getById": 1530,
"hasKey": 1525
},
"all": 8295
}
After memoization reading perf is 50 x slower
With better memoization (I was dumb)
results (ms): {
"writes": {
"total": 1290,
"init": 4,
"set": 34,
"add": 281,
"update": 373,
"remove": 260,
"addOrUpdate": 338
},
"reads": {
"total": 2717,
"getState": 9,
"getKeys": 12,
"getAsArray": 16,
"getLength": 27,
"isInitialized": 19,
"get": 14,
"getByIds": 2539,
"getById": 42,
"hasKey": 39
},
"all": 4007
}
So we run faster with this version memoized since getByIds
is faster
With last commit.
What is your though @bpetetot @frinyvonnick @EmrysMyrddin ?
results (ms): {
"writes": {
"total": 436,
"init": "0.03ms / op [1000]",
"sets": "0.11ms / op [1000]",
"add": "0.24ms / op [1]",
"adds": "0.48ms / op [1000]",
"update": "0.41ms / op [1]",
"updates": "1.38ms / op [1000]",
"remove": "0.3ms / op [1]",
"removes": "0.25ms / op [1000]",
"addOrUpdate": "0.15ms / op [1]",
"addOrUpdates": "1.01ms / op [1000]"
},
"reads": {
"total": 2025,
"getState": "0.00013ms / op [1]",
"getKeys": "0.00015ms / op [1000]",
"getAsArray": "0.00014ms / op [1]",
"getLength": "0.00014ms / op [1]",
"isInitialized": "0.00013ms / op [1]",
"get": "0.00055ms / op [1]",
"getById": "0.00074ms / op [1]",
"getByIds": "0.0177ms / op [1000]",
"hasKey": "0.00057ms / op [1]"
},
"all": 2461
}
This is master:
results (ms): {
"writes": {
"total": 1059,
"init": "0.03ms / op [1000]",
"sets": "0.11ms / op [1000]",
"add": "0.26ms / op [1]",
"adds": "1.42ms / op [1000]",
"update": "1.16ms / op [1]",
"updates": "2.61ms / op [1000]",
"remove": "1.37ms / op [1]",
"removes": "1.04ms / op [1000]",
"addOrUpdate": "0.6ms / op [1]",
"addOrUpdates": "1.99ms / op [1000]"
},
"reads": {
"total": 21410,
"getState": "0.00013ms / op [1]",
"getKeys": "0.00014ms / op [1000]",
"getAsArray": "0.00015ms / op [1]",
"getLength": "0.00016ms / op [1]",
"isInitialized": "0.00015ms / op [1]",
"get": "0.00058ms / op [1]",
"getById": "0.00077ms / op [1]",
"getByIds": "0.20996ms / op [1000]",
"hasKey": "0.00206ms / op [1]"
},
"all": 22469
}
edit:
When reads
occurs the reducer state stores 1505
instances
edit2:
const RERUN_SELECTORS = 100000
const rerunSelectors = callback => Array.from({ length: RERUN_SELECTORS }).reduce(callback)
const map = new Map(Array.from({ length: 1505 }).map((value, index) => [index, { index, object: `value-${index}` }] ))
const timer = Date.now()
console.log(rerunSelectors(() => map.get(292)))
console.log(`${(Date.now() - timer) / RERUN_SELECTORS}ms / op [1]`)
0.00016ms / op [1]
edit3: Don't forget the goals:
(sorry for multiple pings)
I will not fix coverage
Just tested with k-ramel/todo-mvc
and it works well 👍
Will you do the documentation in another PR ?
@frinyvonnick No I will do it here. What do you think about ?
Do you need I publish a new RC on top of this PR ?
@frinyvonnick ?
Do you think its necessary to write some warning about breaking change ?
I don't get it.
simpleObject
)getState
because I couldn't know if the user use it from a new perspective (this PR) or an old (version 5.X.X), I don't want to spam the console with warning everytimes getState
is called.Do you have something specifics in mind ? Ho ! Maybe you speak about README.md ?
Sure i speak about adding some section in README.md like "Migrating from .. to .."
Yes you are right.
I will do this in an other PR then. So this one could be merged and tested in a new RC.
@frinyvonnick do you want to review this PR ?
I wanted to try to limit the state (serialized state) size. So I try to only use the
data
field which is an object.And then use
Object.keys
andObject.values
to retrievearray
andkeys
field, into selectors. I know this is less performant, but I guess this is a good tradeoff for server side rendering (state is tinier in the payload when we want to send the state to the client after the first render -on server-)But but but,
Object.values()
doesn't sort the values to the order defined into the object. So I stop there for now.I let the PR open so the idea still exists.
(FYI @bpetetot @EmrysMyrddin)
fixes #133