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.

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

Refactor construction to avoid new #211

Merged
merged 2 commits into from
Mar 20, 2017
Merged

Refactor construction to avoid new #211

merged 2 commits into from
Mar 20, 2017

Conversation

jsumners
Copy link
Member

I'm not sure if you guys want this. I just wanted to see how Pino would perform if it didn't rely on the new construct. I am attaching single runs of the full benchmark suite from the current master and this refactor branch. It would be nice to know how the runs compare on some other systems. I'll let you be the judge of the results.

There are a lot of changes here. You may want to simply view the full file instead of the diff. I started the work from the issue-209 branch in PR #210.

One nice change here, in my opinion, is that we don't have to expose the _Pino property for things like pino-multi-stream. Those sort of modules can simply get a new instance out of the factory function like normal and do true JavaScript "inheritance".

refactor.bench.txt
master.bench.txt

@mcollina
Copy link
Member

benchmarks are good and just a bit better, just double check but it seems the 'object' one is slightly slower than master. Maybe it was just a bad run, but better check.

If that's ok, 👍 for me.

@jsumners
Copy link
Member Author

I think it's just variations in CPU scheduling. I am attaching the results of three consecutive runs of bench-object on both master and refactor branches. The second run of each shows this. And in each of the three runs the PinoUnsafeExtremeObj results are nearly identical.

bench-object.refactor.txt
bench-object.master.txt

@davidmarkclements
Copy link
Member

oh very nice - I refactored to use constructor because of perf benefits over closure

but if we can use Object.create (which is fine to use now since we're not trying to support v. old JS) instead of new then all the better 👍

Copy link
Member

@davidmarkclements davidmarkclements left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit 0cba928 into master Mar 20, 2017
@jsumners jsumners deleted the refactor branch March 20, 2017 16:46
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants