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

perf: improve performance of getMetricAsString in next #548

Merged
merged 2 commits into from
Mar 9, 2023
Merged

perf: improve performance of getMetricAsString in next #548

merged 2 commits into from
Mar 9, 2023

Conversation

shappir
Copy link
Contributor

@shappir shappir commented Mar 8, 2023

Use Array.prototype.join instead string concatenation for constructing Prometheus metric string. This results in improved performance and reduced memory consumption, especially when there's a large amount of dimensions (labels and label values). In addition uses array iteration methods instead of explicit loops.

note: there are no tests for exemplar so I could not verify that this functionality works as expected.

@SimenB
Copy link
Collaborator

SimenB commented Mar 8, 2023

Wonderful, thank you!

return escapeString(str).replace(/"/g, '\\"');
}
function escapeString(str) {
return str.replace(/\n/g, '\\n').replace(/\\(?!n)/g, '\\\\');
Copy link
Collaborator

Choose a reason for hiding this comment

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

#546 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't go for that. I just simplified that regexp. Can roll back if you prefer

@zbjornson
Copy link
Collaborator

@SimenB I didn't realize you had a "next" branch. Should I be landing all PRs against that? Is that just to keep the README aligned with the current npm release?

I can move the commits I landed from master to next tonight.

@SimenB
Copy link
Collaborator

SimenB commented Mar 8, 2023

@zbjornson yeah, landed removal of old nodes and such. I think landing changes there and just releasing a new major would be nice

@zbjornson
Copy link
Collaborator

@SimenB got it, I will fix up the mess I made and do that going forward.

@SimenB SimenB merged commit 3520f77 into siimon:next Mar 9, 2023
SimenB pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Dan Shappir <dan.s@nextinsurance.com>
SimenB pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Dan Shappir <dan.s@nextinsurance.com>
SimenB pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Dan Shappir <dan.s@nextinsurance.com>
@SimenB
Copy link
Collaborator

SimenB commented Mar 9, 2023

Thanks again @shappir!


@zbjornson I'll just land next on master, then the issue should resolve itself. I think that's easier 😀

SimenB pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Dan Shappir <dan.s@nextinsurance.com>
SimenB pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Dan Shappir <dan.s@nextinsurance.com>
SimenB pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Dan Shappir <dan.s@nextinsurance.com>
@budarin
Copy link

budarin commented Mar 13, 2023

explicit loops are still much more productive than native methods - explicit loops does not have checks for holes in the array and some other checks, so they benefit in performance compared to native array methods

@SimenB
Copy link
Collaborator

SimenB commented Sep 12, 2023

@budarin happy to take a PR to that effect 🙂

@budarin
Copy link

budarin commented Sep 13, 2023

preset-perf 🙂

@SimenB
Copy link
Collaborator

SimenB commented Sep 13, 2023

I don't think we wanna add a compilation step at this time. Cool plugin, tho!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants