Improve processBeaconEvents hotpath
#3200
Conversation
|
Yup the tests were spying on |
| @@ -493,22 +493,24 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap> | |||
| } | |||
| }; | |||
|
|
|||
| events.forEach((event: MatrixEvent) => { | |||
| for (const event of events) { | |||
There was a problem hiding this comment.
TIL that forEach is up to 3 times slower than the classic loops. 😳
There was a problem hiding this comment.
Which makes me wonder if other noticeable performance improvements could be had from banning it elsewhere, too?
There was a problem hiding this comment.
They ran the benchmark with Node 14.
I've tested forEach vs. for of based on their code in the browser.
Interesting results:
Firefox:
forEach of took: 47
for of took: 77
Chromium:
forEach of took: 3807
for of took: 4095
Here it looks like that forEach is the winner.
Code
<!DOCTYPE html>
<html lang="de">
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Titel</title>
</head>
<body>
<div id="results">
</div>
<script>
console.log("Starting");
function generateTestArray() {
const result = [];
for (let i = 0; i < 10000000; ++i) {
result.push({
a: i,
b: i / 2,
r: 0,
});
}
return result;
}
const testArray1 = generateTestArray();
const testArray2 = generateTestArray();
const results = document.getElementById("results");
const before1 = Date.now();
testArray1.forEach((x) => {
x.r = x.a + x.b;
});
const after1 = Date.now();
const before2 = Date.now();
for (const obj of testArray2) {
obj.r = obj.a + obj.b;
}
const after2 = Date.now();
const result1 = document.createElement("DIV");
result1.textContent = "forEach of took: " + (after1 - before1);
results.appendChild(result1);
const result2 = document.createElement("DIV");
result2.textContent = "for of took: " + (after2 - before2);
results.appendChild(result2);
</script>
</body>
</html>
There was a problem hiding this comment.
Oh interesting. Somehow it makes no sense to me that these aren't optimized into the same machine code. 🤔
There was a problem hiding this comment.
Yet another benchmark: https://www.measurethat.net/Benchmarks/Show/18189/0/for-vs-foreach-vs-forin-vs-forof This is more aimed at the overhead of the different ways of iterating an array.
I would conclude: The performance of a loop highly depends on what you do in it.
There was a problem hiding this comment.
It also depends on how the array is interpreted and what it contains.
I found https://v8.dev/blog/elements-kinds interesting to conceptualise some perf optimisation that happen at the JS engine level
I'd optimise for readibility in the source code, and if we found that one thing was really slower than other, looking at transpiling that at build time
There was a problem hiding this comment.
FTR the reason for the swap here was simply for access to async/await
There was a problem hiding this comment.
Yet another benchmark: measurethat.net/Benchmarks/Show/18189/0/for-vs-foreach-vs-forin-vs-forof This is more aimed at the overhead of the different ways of iterating an array.
That particular one showed a factor 100 difference between for of and for in in my browser. I find it mildly irritating that there's so much variation just from the choice of iteration mechanism.
I'd optimise for readibility in the source code, and if we found that one thing was really slower than other, looking at transpiling that at build time
💯
* Allow via_servers property in findPredecessor (update to MSC3946) ([\matrix-org#3240](matrix-org#3240)). Contributed by @andybalaam. * Fire `closed` event when IndexedDB closes unexpectedly ([\matrix-org#3218](matrix-org#3218)). * Implement MSC3952: intentional mentions ([\matrix-org#3092](matrix-org#3092)). Fixes element-hq/element-web#24376. * Send one time key count and unused fallback keys for rust-crypto ([\matrix-org#3215](matrix-org#3215)). Fixes element-hq/element-web#24795. Contributed by @florianduros. * Improve `processBeaconEvents` hotpath ([\matrix-org#3200](matrix-org#3200)). * Implement MSC3966: a push rule condition to check if an array contains a value ([\matrix-org#3180](matrix-org#3180)). * indexddb-local-backend - return the current sync to database promise … ([\matrix-org#3222](matrix-org#3222)). Contributed by @texuf. * Revert "Add the call object to Call events" ([\matrix-org#3236](matrix-org#3236)). * Handle group call redaction ([\matrix-org#3231](matrix-org#3231)). Fixes vector-im/voip-internal#128. * Stop doing O(n^2) work to find event's home (`eventShouldLiveIn`) ([\matrix-org#3227](matrix-org#3227)). Contributed by @jryans. * Fix bug where video would not unmute if it started muted ([\matrix-org#3213](matrix-org#3213)). Fixes element-hq/element-call#925. * Fixes to event encryption in the Rust Crypto implementation ([\matrix-org#3202](matrix-org#3202)).
Improved test by @ara4n from 4230ms to 6.2ms spent in
processBeaconEvents()O(n)task intoO(n log2 n)- made worse by a GC taking 10% extra timeIf we had a map storing the beaconInfoId -> Beacon we could make this even more efficient, given the data access pattern of every subsequent sync needing access to such a lookup it seems strange we make each /sync build one from scratch
Here's what your changelog entry will look like:
✨ Features
processBeaconEventshotpath (#3200).