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

🐛 utils: fix EqualFold and docs #1833

Merged
merged 2 commits into from Mar 23, 2022
Merged

🐛 utils: fix EqualFold and docs #1833

merged 2 commits into from Mar 23, 2022

Conversation

jfcg
Copy link
Contributor

@jfcg jfcg commented Mar 22, 2022

My original assumptions were wrong, but along the way I improved the tests/benchmarks and fixed a bug in EqualFold.

@ReneWerner87
Copy link
Member

Please provide before and after benchmark results.

utils/strings.go Show resolved Hide resolved
@jfcg jfcg changed the title ⚡ improve utils.ToLower/ToUpper, remove EqualFold 🐛 utils: fix EqualFold and docs Mar 22, 2022
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

sorry but cannot accept
before(master):

Benchmark_EqualFoldBytes/fiber-12         	35822534	        34.25 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/fiber-12         	33736502	        34.00 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/fiber-12         	35774396	        34.25 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/fiber-12         	35762599	        33.62 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	11068182	       108.7 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	10881543	       106.4 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	11284395	       119.6 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	 9314128	       112.8 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	35246314	        38.88 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	30589214	        36.25 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	31582516	        34.62 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	35663390	        33.86 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	12866125	        95.12 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	12907418	        94.02 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	12663051	        94.05 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	12583516	        93.54 ns/op	       0 B/op	       0 allocs/op

after(your branch):

Benchmark_EqualFoldBytes/fiber-12         	13826067	        76.53 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/fiber-12         	15962703	        76.39 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/fiber-12         	15975374	        75.84 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/fiber-12         	15986911	        75.67 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	 5385681	       223.6 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	 4988062	       234.4 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	 5206946	       225.8 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFoldBytes/default-12       	 5001892	       226.4 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	19036320	        63.21 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	19063516	        62.18 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	18857341	        62.93 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/fiber-12              	19338664	        62.21 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	 6329773	       212.9 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	 5200621	       199.7 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	 5531955	       224.3 ns/op	       0 B/op	       0 allocs/op
Benchmark_EqualFold/default-12            	 6175377	       202.6 ns/op	       0 B/op	       0 allocs/op

@ReneWerner87
Copy link
Member

i like the comment change and the test change, but not the performance drop

if you can customize that i would merge it

next time please share the benchmark results directly

since fiber stands for speed, we always have to look at the benchmarks, because the users trust us to always get the best possible out of it

@jfcg
Copy link
Contributor Author

jfcg commented Mar 23, 2022

@ReneWerner87, see the definition of largeStr. It is about twice length of the previous input. That's the reason, I have not changed ToUpper and ToLower functions, see the updated patch. The change in EqualFold is mandatory because there was a bug in it. Also see the extra tests for EqualFold.
Thanks..

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 23, 2022

okay i see, so in any case 7ns for the fix of the bug come on it

tested it again with the original strings

is there a reason why you do the comparison from back to front ?

@jfcg
Copy link
Contributor Author

jfcg commented Mar 23, 2022

Reverse iteration produces one less opcode on x86 for a good compiler, old trick, but irrelevant for modern CPUs today. It could be either way.

@ReneWerner87
Copy link
Member

Can you also create a PR for https://github.com/gofiber/utils

@ReneWerner87 ReneWerner87 merged commit 2b5b6b2 into gofiber:master Mar 23, 2022
@jfcg jfcg deleted the improve-upper-lower branch March 23, 2022 16:30
@jfcg
Copy link
Contributor Author

jfcg commented Mar 24, 2022

Hi @ReneWerner87, is gofiber/utils repo an outdated version of fiber/utils directory?

@ReneWerner87
Copy link
Member

I had the idea to use the functions which have nothing to do with the web server even if you are in a project without a web server.

@ReneWerner87
Copy link
Member

Hi @ReneWerner87, is gofiber/utils repo an outdated version of fiber/utils directory?

Yes

@jfcg
Copy link
Contributor Author

jfcg commented Mar 25, 2022

@ReneWerner87 see the discussion here.

trim21 pushed a commit to trim21/fiber that referenced this pull request Aug 15, 2022
* 🔍 utils: add/improve tests for ToLower/ToUpper/EqualFold

* 🐛 utils: fix EqualFold and docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants