Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(serializers): properly transfer Symbol based serializers #707

Merged
merged 4 commits into from
Sep 13, 2019
Merged

fix(serializers): properly transfer Symbol based serializers #707

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
797ad38
fix(serializers): properly transfer Symbol based serializers
MickVanDuijn Sep 11, 2019
b1e303e
chore: do not use array spread
MickVanDuijn Sep 11, 2019
78efd50
chore: replace for-of by for(;;)
MickVanDuijn Sep 11, 2019
be84ca8
chore: extended Symbol serializer test
MickVanDuijn Sep 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/proto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ function child (bindings) {
const instance = Object.create(this)
if (bindings.hasOwnProperty('serializers') === true) {
instance[serializersSym] = Object.create(null)
for (var k in serializers) {
for (var k of [...Object.getOwnPropertySymbols(serializers), ...Object.keys(serializers)]) {
instance[serializersSym][k] = serializers[k]
}
for (var bk in bindings.serializers) {
for (var bk of [...Object.getOwnPropertySymbols(bindings.serializers), ...Object.keys(bindings.serializers)]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will definitely be slower, even with a for loop. for in is still the fastest way to loop through key names

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split the loops into two parts: a for-in on the keys, plus a for(;;) on the symbols

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

instance[serializersSym][bk] = bindings.serializers[bk]
}
} else instance[serializersSym] = serializers
Expand Down
24 changes: 24 additions & 0 deletions test/serializers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,30 @@ test('children inherit parent serializers', async ({ is }) => {
is(o.test, 'parent')
})

test('children inherit parent Symbol serializers', async ({ is, isNot }) => {
const stream = sink()
const symbolSerializers = {
[Symbol.for('pino.*')]: parentSerializers.test
}
const parent = pino({ serializers: symbolSerializers }, stream)

is(parent[Symbol.for('pino.serializers')], symbolSerializers)

const child = parent.child({
serializers: {
a
}
})

function a () {
return 'hello'
}

isNot(child[Symbol.for('pino.serializers')], symbolSerializers)
is(child[Symbol.for('pino.serializers')].a, a)
is(child[Symbol.for('pino.serializers')][Symbol.for('pino.*')], parentSerializers.test)
})

test('children serializers get called', async ({ is }) => {
const stream = sink()
const parent = pino({
Expand Down