[pr] minimize log message string allocations #117
Conversation
stephen-palmer
commented
Apr 12, 2019
stephen-palmer
commented
- short-circuit debug logging logic when logging level is below debug, to eliminate associated string allocations
- in non-cluster mode, eliminate double allocations of log message strings by removing redundant template literal
- in cluster mode, use cached string prefixes and eliminate double allocations of log message strings
- eliminated use of the LOG_TEST level in production code. Just a handful of log lines were using it - they were redistributed to INFO or DEBUG.
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here - or - cancel by adding [!pr] to the title of the pull request.
lib/helpers.js
Outdated
| if (lvl <= logLevel) { | ||
| const prefix = cluster.isMaster ? "[Cluster:M] " : `[Cluster:${cluster.worker.id}] `; | ||
| console.log(`${prefix}${msg}`); | ||
| if (!shouldLog(lvl)) return; |
import.js
Outdated
| @@ -74,7 +74,7 @@ async function importTransactionFile(filePath, addressString, defaultPort) { | |||
| } | |||
|
|
|||
| try { | |||
| helpers.log(consts.LOG_DBG, `Begin transaction for ${helpers.GUIDBufferToString(guid)}-${hash.toString('hex')}`); | |||
| helpers.shouldLog(consts.LOG_DBG) && helpers.log(consts.LOG_DBG, `Begin transaction for ${helpers.GUIDBufferToString(guid)}-${hash.toString('hex')}`); | |||
There was a problem hiding this comment.
Have you tried to see the memory implications of using an arrow function to wrap the strings? I assume you probably did try that. If that worked it might make this a little cleaner and prevent having to repeat the log level twice (it'll be easy to accidentally change one without changing the other in the future).
If the arrow function is never called because of the log level the string wouldn't be evaluated. But I'm not sure how the memory usage compares to what you're currently experiencing. It would be something I would try if you haven't already.
There was a problem hiding this comment.
This is a good suggestion - I'm going to look into it. I'll probably write a little test app to profile memory using the two approaches, against the baseline.
|
I did some profiling on this and found that the string templates are actually not processed/de-referenced until they are used by console.log() ... therefore this optimization is largely unnecessary. There are a couple of things in this PR I want to keep regardless. Separately, in the process, I found a more important optimization that can be done to specifically address performance and memory usage when downloading a very large number of files. I'll open a separate PR for that one. |
