fix(serializers): properly transfer Symbol based serializers #707
Conversation
There was a problem hiding this comment.
Would you mind running https://github.com/pinojs/pino/blob/master/benchmarks/child-creation.bench.js before/after? I would just like to make sure that we are not losing performance on child creation.
lib/proto.js
Outdated
| instance[serializersSym][k] = serializers[k] | ||
| } | ||
| for (var bk in bindings.serializers) { | ||
| for (var bk of [...Object.getOwnPropertySymbols(bindings.serializers), ...Object.keys(bindings.serializers)]) { |
There was a problem hiding this comment.
Can you create only 1 instance of these arrays? Given that we will be iterating an array now, I prefer we use a standard for (;;) loop instead, as it's faster than for-of.
There was a problem hiding this comment.
this will definitely be slower, even with a for loop. for in is still the fastest way to loop through key names
There was a problem hiding this comment.
split the loops into two parts: a for-in on the keys, plus a for(;;) on the symbols
There was a problem hiding this comment.
timings before this PR:
benchBunyanCreation*10000: 574.174ms
benchBoleCreation*10000: 233.600ms
benchPinoCreation*10000: 241.948ms
benchPinoExtremeCreation*10000: 131.857ms
benchPinoNodeStreamCreation*10000: 250.361ms
benchBunyanCreation*10000: 544.266ms
benchBoleCreation*10000: 225.199ms
benchPinoCreation*10000: 244.614ms
benchPinoExtremeCreation*10000: 127.672ms
benchPinoNodeStreamCreation*10000: 228.082ms
timings after this PR:
benchBunyanCreation*10000: 597.158ms
benchBoleCreation*10000: 241.598ms
benchPinoCreation*10000: 239.223ms
benchPinoExtremeCreation*10000: 134.678ms
benchPinoNodeStreamCreation*10000: 237.142ms
benchBunyanCreation*10000: 510.368ms
benchBoleCreation*10000: 200.756ms
benchPinoCreation*10000: 216.045ms
benchPinoExtremeCreation*10000: 119.050ms
benchPinoNodeStreamCreation*10000: 209.216ms
they seem to heavily fluctuate between runs, but there is no major difference.
Note: the child benchmarks don't include serializers, so this loop is never hit in those benchmarks
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
When adding serializers through
.child, the Symbol based serializers were not transferred. For example: https://github.com/pinojs/pino/blob/master/docs/api.md#serializerssymbolforpino-functionThanks to @ZwaarContrast for helping with debugging!