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

feat: add errorKey option #1507

Merged
merged 2 commits into from
Aug 4, 2022
Merged

feat: add errorKey option #1507

merged 2 commits into from
Aug 4, 2022

Conversation

qwelias
Copy link
Contributor

@qwelias qwelias commented Jul 26, 2022

We have custom messageKey and serializers, makes sense to have custom errorKey as well.
Not sure if that should affect browser part

@mcollina
Copy link
Member

There seem to be a significant decrease of throughput in our benchmarks.

Current:

benchPino*10000: 148.753ms

this branch:

benchPino*10000: 166.715ms

@mcollina
Copy link
Member

CI is failing too.

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

Changes have minimal logic, so I don't think there's any room for perf improvement.
They don't affect any transport functionality either (and certainly nothing platform-specific), I assume tests may be flaky, may need a rerun

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

@mcollina
How did you run your benchmarks? 1st few runs are usually faster because CPU is cold. Make sure to do a few consecutive runs on each branch to avoid CPU throttling bias.

For me both versions run bench-basic with Pino average in 96-104 (master actually peaked at 107 once, so even slower, did about dozen runs each)

@jsumners
Copy link
Member

@mcollina How did you run your benchmarks? 1st few runs are usually faster because CPU is cold. Make sure to do a few consecutive runs on each branch to avoid CPU throttling bias.

For me both versions run bench-basic with Pino average in 96-104 (master actually peaked at 107 once, so even slower, did about dozen runs each)

See https://github.com/pinojs/pino/runs/7528251758?check_suite_focus=true

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

@jsumners
I can see no significant difference:

  • benchmark branch BASIC benchPino*10000: 183.329ms
  • benchmark current BASIC benchPino*10000: 188.812ms

So idk where @mcollina took the numbers from

@mcollina
Copy link
Member

we run the benchmarks for every CI run

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

@mcollina which run did you take numbers from?

@mcollina
Copy link
Member

the first in this PR

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

@mcollina
I'm sorry, but I see benchPino*10000: 188.812ms here's link (vs benchmark branch).

Can you link yours? I literally have no idea where you see those numbers, search doesn't find them either

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

Thanks!
But why are they called the same!?

@mcollina
Copy link
Member

There is still a difference.

@qwelias
Copy link
Contributor Author

qwelias commented Jul 27, 2022

What's average margin of error? Is the difference more than average?
Is it significant to block the feature?

@mcollina
Copy link
Member

I tested on a dedicated workstation, and there is no perceptible slowdown. We can land.

@mcollina
Copy link
Member

Don't worry about the flaky test.

Copy link
Member

@mcollina mcollina 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 b288da5 into pinojs:master Aug 4, 2022
@github-actions
Copy link

github-actions bot commented Aug 5, 2023

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 Aug 5, 2023
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.

3 participants