some loop don't need to transfer to bytes slice and some use the zero… #2572

mask-pp · 2020-12-01T11:27:28Z

some loop don't need to transfer to bytes slice and some use the zero copy will much better.

… copy will much better.

codecov · 2020-12-02T07:18:01Z

Codecov Report

Merging #2572 (6d51ad2) into master (7742ff5) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2572      +/-   ##
==========================================
- Coverage   98.48%   98.47%   -0.01%     
==========================================
  Files          41       41              
  Lines        1974     1973       -1     
==========================================
- Hits         1944     1943       -1     
  Misses         17       17              
  Partials       13       13

Impacted Files	Coverage Δ
tree.go	`100.00% <100.00%> (ø)`

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 7742ff5...6d51ad2. Read the comment docs.

.gitignore

thinkerou · 2020-12-03T08:06:38Z

tree.go

@@ -611,7 +611,7 @@ walk: // Outer loop for walking the tree
 				if rb[0] != 0 {
 					// Old rune not finished
 					idxc := rb[0]
-					for i, c := range []byte(n.indices) {
+					for i, c := range bytesconv.StringToBytes(n.indices) {


why add it here, but not add before?

loop string type and don't cpompare with a byte don't need to do it, but that loop used so change to slice is necessary, and use StringToBytes will be better. Thanks!

thanks. using local var save the result of StringToBytes?

I believe it's unnecessary to call bytesconv.StringToBytes in a for range loop, there is a compiler optimization that will avoid the copies, see https://github.com/golang/go/wiki/CompilerOptimizations#range-over-bytes.

ok, i see thanks.

thanks. using local var save the result of StringToBytes?

that kind of change is not necessary.

thinkerou · 2020-12-03T12:00:34Z

@appleboy @panjf2000 please help review the pull request, thanks!

panjf2000 · 2020-12-03T12:11:33Z

internal/bytesconv/bytesconv.go

-	bh := (*reflect.SliceHeader)(unsafe.Pointer(&b))
-	bh.Data, bh.Len, bh.Cap = sh.Data, sh.Len, sh.Len
-	return b
+func StringToBytes(s string) []byte {


What's the theorem behind this change?

@panjf2000 based on #2553, we also need to change it.

tree.go

thinkerou · 2020-12-04T05:54:07Z

unit test error:

=== RUN   TestRenderAsciiJSON
    render_test.go:173: 
        	Error Trace:	render_test.go:173
        	Error:      	Not equal: 
        	            	expected: "{\"lang\":\"GO\\u8bed\\u8a00\",\"tag\":\"\\u003cbr\\u003e\"}"
        	            	actual  : "{\"lang\":\"GO\\u00e8\\u00af\\u00ad\\u00e8\\u00a8\\u0080\",\"tag\":\"\\u003cbr\\u003e\"}"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-{"lang":"GO\u8bed\u8a00","tag":"\u003cbr\u003e"}
        	            	+{"lang":"GO\u00e8\u00af\u00ad\u00e8\u00a8\u0080","tag":"\u003cbr\u003e"}
        	Test:       	TestRenderAsciiJSON
--- FAIL: TestRenderAsciiJSON (0.00s)

mask-pp · 2020-12-04T06:17:04Z

sorry, it's a mistake, should use utf8 type but not the byte.

thinkerou · 2020-12-11T02:40:54Z

tree.go

 				// []byte for proper unicode char conversion, see #65
-				n.indices += bytesconv.BytesToString([]byte{c})
+				n.indices += path[0:1]


here maybe we should add it, thanks!

thinkerou · 2020-12-11T02:41:39Z

@appleboy please review the pull request, thanks!

appleboy · 2020-12-11T05:31:08Z

@mask-pp Please provide the benchmark report?

mask-pp · 2020-12-11T09:24:15Z

@mask-pp Please provide the benchmark report?

ok, Is about StringToBytes change or others?

appleboy · 2020-12-11T11:42:13Z

Provide the benchmark result. See https://github.com/gin-gonic/gin/blob/master/BENCHMARKS.md

thinkerou · 2020-12-23T01:38:10Z

any update? Thanks

mask-pp · 2020-12-23T06:10:21Z

any update? Thanks

already updated bentchmark report, please check. thanks!

appleboy · 2020-12-23T07:33:42Z

@mask-pp upload the benchmark in the comment not update the markdown file benchmarkd.md

mask-pp · 2020-12-23T10:23:02Z

Provide the benchmark result. See https://github.com/gin-gonic/gin/blob/master/BENCHMARKS.md

thinkerou · 2020-12-29T01:46:52Z

why closed?

mask-pp added 3 commits December 1, 2020 19:24

some loop don't need to transfer to bytes slice and some use the zero…

c10029a

… copy will much better.

update gitignore

0d41d35

upgrade StringToBytes function

ae39f91

thinkerou reviewed Dec 3, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

thinkerou requested review from appleboy and thinkerou December 3, 2020 07:46

thinkerou added this to the 1.7 milestone Dec 3, 2020

thinkerou added the enhancement label Dec 3, 2020

thinkerou reviewed Dec 3, 2020

View reviewed changes

panjf2000 reviewed Dec 3, 2020

View reviewed changes

tree.go Outdated Show resolved Hide resolved

panjf2000 reviewed Dec 3, 2020

View reviewed changes

tree.go Outdated Show resolved Hide resolved

mask-pp added 3 commits December 4, 2020 09:27

revort unnecessary change

5ff5000

revort unnecessary change

392260a

simplify code

75021e2

revort a mistake operation

8edf824

thinkerou reviewed Dec 11, 2020

View reviewed changes

update bentchmark report

6d51ad2

mask-pp closed this Dec 23, 2020

appleboy pushed a commit that referenced this pull request Jan 3, 2021

fix compile error from #2572 (#2600)

3b5e861

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

some loop don't need to transfer to bytes slice and some use the zero… #2572

some loop don't need to transfer to bytes slice and some use the zero… #2572

mask-pp commented Dec 1, 2020

codecov bot commented Dec 2, 2020 •

edited

Loading

thinkerou Dec 3, 2020

mask-pp Dec 3, 2020

thinkerou Dec 3, 2020

panjf2000 Dec 3, 2020

mask-pp Dec 4, 2020

mask-pp Dec 4, 2020

thinkerou commented Dec 3, 2020

panjf2000 Dec 3, 2020 •

edited

Loading

thinkerou Dec 3, 2020

thinkerou commented Dec 4, 2020

mask-pp commented Dec 4, 2020

thinkerou Dec 11, 2020

thinkerou commented Dec 11, 2020

appleboy commented Dec 11, 2020

mask-pp commented Dec 11, 2020

appleboy commented Dec 11, 2020

thinkerou commented Dec 23, 2020

mask-pp commented Dec 23, 2020

appleboy commented Dec 23, 2020

mask-pp commented Dec 23, 2020

thinkerou commented Dec 29, 2020

some loop don't need to transfer to bytes slice and some use the zero… #2572

some loop don't need to transfer to bytes slice and some use the zero… #2572

Conversation

mask-pp commented Dec 1, 2020

codecov bot commented Dec 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkerou commented Dec 3, 2020

panjf2000 Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkerou commented Dec 4, 2020

mask-pp commented Dec 4, 2020

Choose a reason for hiding this comment

thinkerou commented Dec 11, 2020

appleboy commented Dec 11, 2020

mask-pp commented Dec 11, 2020

appleboy commented Dec 11, 2020

thinkerou commented Dec 23, 2020

mask-pp commented Dec 23, 2020

appleboy commented Dec 23, 2020

mask-pp commented Dec 23, 2020

thinkerou commented Dec 29, 2020

codecov bot commented Dec 2, 2020 •

edited

Loading

panjf2000 Dec 3, 2020 •

edited

Loading