-
-
Notifications
You must be signed in to change notification settings - Fork 55
test: add benchmark for log.fromEntryHash() #138
test: add benchmark for log.fromEntryHash() #138
Conversation
56c2d71
to
c73ffaa
Compare
await log.append('hello' + i, refCount) | ||
process.stdout.write("\rWriting " + i + " / " + count) | ||
} | ||
const dt1 = new Date().getTime() |
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.
Would be nice to align the indentation here and the lines above to match the try/catch scope.
|
||
// total = result.length | ||
// queriesPerSecond = result.length | ||
// queriesPerSecond = totalQueries - queriesPerSecond |
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.
Before merging, we should remove all the commented lines.
console.log("Loading took:", (et - dt2), "ms") | ||
const memory = process.memoryUsage() | ||
console.log(`Memory Heap Used: ${memory.heapUsed / 1024 / 1024} MB`) | ||
console.log(`Memory Heap Total: ${memory.heapTotal / 1024 / 1024} MB`) |
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.
Very cool to output the memory usage! 👍
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.
Would it make sense to snapshot/capture the heap used also before the benchmark is run and do a delta comparison (ie. what is the differences between before and after)?
if (lastTenSeconds === 0) throw new Error('Problems!') | ||
lastTenSeconds = 0 | ||
} | ||
console.log(`\n${queriesPerSecond} queries per second, ${totalQueries} queries in ${seconds} seconds (Entry count: ${total})`) |
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.
If I read the code correctly, we're benchmarking "how long does it take to load a log" and as such we're not necessarily doing queries, but "entries per second". I feel it'd be worthwhile to make the output messages clearer on that and display something like "X entries loaded per second" etc.
What do you think?
If you feel that would improve the PR, we should also rename the variables used in the code to match the output, eg. queriesPerSecond
-> entriesLoadedPerSecond
or something similar.
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.
No brainer change - lazy of me to have not done it before 😁
Thank you @mistakia, this looks good to me 👍 |
Let me know if you prefer consolidating these benchmarks to one PR.
Also, let me know if you have any suggestions on how to approach memory heap usage profiling.
Related: #136