-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the attributes reducer to use a map instead of a regular object #46146
Conversation
Size Change: +1.51 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
The performance test constructs a map (with two implementations, old and new) with 100k entries and then performs The most expensive operation seems to be copying the map. Old way is: const newState = { ...oldState };
newState[ clientId ] = attributes; The new way is: const newState = new Map( oldState );
newState.set( clientId, attributes ); The performance results say that copying the Wondering about two things:
|
Yes, because it's the actual action that is triggered when you type and it's the most important metric for us.
I was wondering the same, I'd assume right now we only support browsers that support this natively but I may be wrong. I'll see if I can spot it in the built files.
Potentially, this sounds like something that needs a dedicated library. Immutable.js looks too heavy to me. Do you have any suggestions here? I tried "Immer" as it's also a popular library in Redux land, but it turned out be way less performant. |
Confirmed this, it's not transpiled, it uses native spreading in built files. (I'm assuming that jest is using the same config though) |
I did the same test again with 2000 blocks (an example that is more likely to be found in real life). Here are the results:
|
I tried to do a local benchmark in plain Node.js with no Jest and no potential transpilation, and the results are interesting: native object spread is (by far) the fastest, map is in the middle, and This is the test script: const k = (i) => i + "";
const v = (i) => ({ content: "post " + i });
function test(body) {
let value;
const s = new Date();
for (let j = 0; j < 4000; j++) {
value = body(j, value);
}
const e = new Date();
return e - s;
}
function objAssign(j, o = {}) {
o = Object.assign({}, o);
o[k(j)] = v(j);
return o;
}
function objSpread(j, o = {}) {
o = { ...o };
o[k(j)] = v(j);
return o;
}
function mapCopies(j, m = new Map()) {
m = new Map(m);
m.set(k(j), v(j));
return m;
}
for (let k = 0; k < 3; k++) {
console.log("obj assign", test(objAssign));
console.log("obj spread", test(objSpread));
console.log("map copies", test(mapCopies));
console.log();
} and the output:
|
Summoning the wisdom of @sgomes because if I recall correctly, he had an opinion about that kind of benchmarking 😉 |
@jsnajdr I think the test you wrote is slightly incorrect in the sense that it tests both "set" and "assign" at the same going from an empty object to a full object. What we care about instead is to run the body of your test (same bodies) but after the object contains already a high number of elements. Basically, this corresponds to typing in a large post. In other words, something like this
and here are the results I got
map is always faster aside the last attempt (I suspect something related to garbage collection but I'm not sure). Edit: if you increase the number of attempts (from 3 to say 10), you'll see that map is always faster except maybe for one of the iterations. |
Thanks, @tyxla, I do! 😄 In general, micro-benchmarks like that are not as useful as we'd like them to be for a number of reasons, but the two most important are:
Then there's also the important matter that we need to make sure that the thing we're testing is in fact the hot path! It seems that some due diligence has been made here, but we should probably go further and identify the slow line in a profiling run, and make sure that it's the one we expect. Testing should always be done as close as possible to a real-world scenario, so I think the initial strategy of using a performance unit test is the best, even though it's a somewhat artificial test. When testing A/B changes like this, however, we need to ensure that the results are statistically significant. Ideally, that means multiple runs, setting up the hypothesis that the new version is faster, and using a Welch's t-test to ensure that the p-value of that hypothesis is significant. I wrote a tool that does that for a bunch of different metrics on a page load. It's probably overkill to do the work of applying it here, but I'd be happy to help validate this fix with some perf measurements if you're interested. |
I updated the test to create an initial object/map with 2000 entries, and then benchmark adding the 2001st entry. Your test does the addition only once, or maybe 3 or 5 times. I ran it 1000 times, always adding one entry to the original 2000-sized object. The results still favor native spread:
Check out the updated script: const k = (i) => i + "";
const v = (i) => ({ content: "post " + i });
const initialObj = {};
const initialMap = new Map();
console.log('initializing...')
for (let i = 0; i < 2000; i++) {
initialObj[k(i)] = v(i);
initialMap.set(k(i), v(i));
}
function test(add, value) {
const s = new Date();
for (let j = 0; j < 1000; j++) {
add(value, 2001);
}
const e = new Date();
return e - s;
}
function objAssign(o, j) {
return Object.assign({}, o, { [k(j)]: v(j) });
}
function objSpread(o, j) {
return { ...o, [k(j)]: v(j) };
}
function mapCopies(m, j) {
m = new Map(m);
m.set(k(j), v(j));
return m;
}
console.log('testing...');
for (let k = 0; k < 3; k++) {
console.log();
console.log("obj assign", test(objAssign, initialObj));
console.log("obj spread", test(objSpread, initialObj));
console.log("map copies", test(mapCopies, initialMap));
} |
@jsnajdr I see you're still looping 1000 times inside the test function (between the dates) so I think that's not really close to what happens when you type. |
Yes, I want to take 1000 measurements and calculate their average. In every loop iteration, I'm adding one new entry to an existing object with 2000 entries. It's not like adding 1000 new entries to the same object, I'm always starting from the original object. That's like doing 1000 keypresses when typing, isn't it? When I type, every keystroke means adding/updating just one entry (the current block) in the large map, is that right? I want to avoid measuring a single event that takes only 1-3 ms to finish. That's too imprecise, too much noise. That's all. |
I understand but the way these JS engines work is weird and doing 1000 similar operations in a row make the engines trigger optimizations that favor an approach over another making it unrealistic in real life. That's why running only 1 addition is IMO more accurate. |
If you want bigger numbers, maybe you can use a bigger initial state (bigger array, map) |
This is correct. Chromium/V8, for example, has various stages of optimisation that only kick in at certain thresholds. If the goal is to increase confidence by repeating a test, it should be done with multiple runs, rather than a loop within a single run. |
OK, I did that, and have some interesting results 🙂 I increased the size of the initial object/map to 50000 entries, and am measuring the addition of just one 50001st entry. I also switched from function test(add, create) {
const measurements = [];
for (let i = 0; i < 15; i++) {
const value = create();
const s = performance.now();
add(value, 50001);
const e = performance.now();
measurements.push(e - s);
}
return measurements;
} The results are:
All three methods have approximately the same speed at the beginning, about 10ms to perform the addition. But in further iterations it gets interesting:
Apparently it's the function objSpread(o, j) {
return { ...o, [k(j)]: v(j) };
} that gets optimized. What else could it be? We are creating a brand new initial object on every run.
If it's really the spread operation that gets optimized after 10 invocations, then that's relevant for the real-life Gutenberg. The reducers are called all the time, and if they get optimized after a few runs, we are getting that improvement in real usage. |
And to test this hypothesis and get some real numbers, we could run a more real-world test using the real reducers, and actually the whole state framework, rather than a synthetic JS benchmark 🙂 |
That's a cool idea! We could take the whole A typical reducer is just a few spread operators nested in each other, so the optimization we're seeing should be preserved. |
Isn't this what my test is doing? |
Oh yes, this test got evicted from my LRU memory cache over the course of the day 🙂 |
@jsnajdr the brain's micro-optimizations can be tricky :) |
I ran this reducer performance test 20 times in a row now, to see if repeated calls get optimized. For both versions, making sure the spread operator is native. And unfortunately the optimization is not there. Results for the original object map (20 timings in ms):
And the results for
|
Thanks for checking @jsnajdr I guess we can move forward with this PR then? As a follow-up, I want to also see if we can make further improvements by updating other similar reducers to maps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it's really a realistic speed improvement 🙂
Sorry for the delayed response @youknowriad. Looks like the mobile unit tests have finally succeeded 🎊 , is there anything else that we'd need to take a look at from the mobile side? Thanks! |
@fluiddot Thanks for chiming in, it's all good I think. |
...getFlattenedBlockAttributes( action.blocks ), | ||
}; | ||
case 'INSERT_BLOCKS': { | ||
const newState = new Map( state ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad I'm inclined to add a comment documenting the choice of operations for a tangible performance hot-path optimization here. It'd be easy to lose track of it in a future refactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are reasons why we normally don't put Map
s or other non-serializable values in the Redux store. It's no longer showing the state in the Redux devtools for me after this PR.
I tried "Immer" as it's also a popular library in Redux land, but it turned out be way less performant.
I wonder if there's any benchmark on this? Seems like Immer should also be performant in most cases. Were we testing it in the production build?
(Side note: adopting Immer should also help us when we want to integrate yjs
more closely into the system for collaborative editing.)
I think we should at least bring back the ability to debug values in the Redux devtools. Maybe the serialize
option would help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any specific reason why Maps should be avoided, other than Redux DevTools not working with them (which to me sounds like a Redux DevTools problem)?
The link only goes on to mention:
"It also ensures that the UI will update as expected."
I'm not sure which UI this refers to, nor why it would stop updating as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's any benchmark on this?
It's a benchmark I did on my own using the same test included in this PR. A refactor using immer is actually very simple but it was three times worse in terms of performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call on the "serialize" thing, I'll be fixing that in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up here #46282
What and why?
While profiling the typing performance, I noticed that the block editor reducer is taking some milliseconds there, so I added a "performance.js" unit test to try to check how the reducer performance is impacted by a high number of blocks. With no surprise, the more blocks we have, the slower the reducer gets, so my conclusion was that since this is being called on each type synchronously, we need to make sure the reducer is as fast as possible. This PR is a first step.
How?
I noticed that the reducer time is almost entirely spent on object destructuring, so I tried several approaches here and one that seemed the most impactful was using maps instead of objects. As it stands the PR only updates the "attributes" sub reducer to use maps, but I suspect that all block related reducer would benefit from using maps.
Here are the results of my small benchmark (which I included in the commit)
While I'm not sure how much impact this will have on the typing performance (we'll see), I believe we should probably update all these blocksByClientId reducer states to use maps instead of objects.