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

Speed up NextLabel and PrevLabel #1039

Merged
merged 7 commits into from
Dec 4, 2019
Merged

Speed up NextLabel and PrevLabel #1039

merged 7 commits into from
Dec 4, 2019

Conversation

jpicht
Copy link
Contributor

@jpicht jpicht commented Nov 29, 2019

NextLabel and PrevLabel are heavily used in the "file" plugin in coredns. This change introduces benchmark tests and improves the performance (on my machine).

It eliminates all allocations and uses strings in a way that should make L1-Cache-Hits more likely.

The time for coredns "file"-plugin Requests is cut roughly in half.

@codecov-io
Copy link

codecov-io commented Nov 29, 2019

Codecov Report

Merging #1039 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1039      +/-   ##
=========================================
+ Coverage   54.92%     55%   +0.07%     
=========================================
  Files          41      41              
  Lines        9869    9879      +10     
=========================================
+ Hits         5421    5434      +13     
+ Misses       3425    3423       -2     
+ Partials     1023    1022       -1
Impacted Files Coverage Δ
labels.go 95.57% <100%> (+2.37%) ⬆️
server.go 67.67% <0%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b7437f...0f96db1. Read the comment docs.

Copy link
Collaborator

@tmthrgd tmthrgd left a comment

Choose a reason for hiding this comment

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

LGTM modulo some minor changes. I wanted to do this a while back, but never got around to it.

labels.go Outdated Show resolved Hide resolved
labels.go Outdated Show resolved Hide resolved
labels.go Show resolved Hide resolved
labels.go Outdated Show resolved Hide resolved
labels.go Show resolved Hide resolved
labels.go Show resolved Hide resolved
@miekg
Copy link
Owner

miekg commented Dec 4, 2019

thank you both

@miekg miekg merged commit 22cda6d into miekg:master Dec 4, 2019
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
* change NextLabel and PrevLabel to be faster

This reduces readability, but they are in the hot path of coredns.

* @redyeti pointed out, that my implementation disregarded triple backslashes

* add synthetic benchmark-tests for PrevLabel and NextLabel

* rename ii -> j

* invert compare

* PrevLabel: add empty string check + test case

* NextLabel: fix and add testcase for NextLabel("", offset>0)
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