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

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

Closed
wants to merge 8 commits into from

Conversation

@mask-pp
Copy link

@mask-pp mask-pp commented Dec 1, 2020

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

@codecov
Copy link

@codecov codecov bot commented Dec 2, 2020

Codecov Report

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

Impacted file tree graph

@@            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 Outdated Show resolved Hide resolved
@thinkerou thinkerou requested review from appleboy and thinkerou Dec 3, 2020
@thinkerou thinkerou added this to the 1.7 milestone Dec 3, 2020
tree.go Outdated
@@ -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) {
Copy link
Member

@thinkerou thinkerou Dec 3, 2020

why add it here, but not add before?

Copy link
Author

@mask-pp mask-pp Dec 3, 2020

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!

Copy link
Member

@thinkerou thinkerou Dec 3, 2020

thanks. using local var save the result of StringToBytes?

Copy link
Contributor

@panjf2000 panjf2000 Dec 3, 2020

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.

Copy link
Author

@mask-pp mask-pp Dec 4, 2020

ok, i see thanks.

Copy link
Author

@mask-pp mask-pp Dec 4, 2020

thanks. using local var save the result of StringToBytes?

that kind of change is not necessary.

@thinkerou
Copy link
Member

@thinkerou thinkerou commented Dec 3, 2020

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

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 {
Copy link
Contributor

@panjf2000 panjf2000 Dec 3, 2020

What's the theorem behind this change?

Copy link
Member

@thinkerou thinkerou Dec 3, 2020

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

tree.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
@thinkerou
Copy link
Member

@thinkerou thinkerou commented Dec 4, 2020

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
Copy link
Author

@mask-pp mask-pp commented Dec 4, 2020

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

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

@thinkerou thinkerou Dec 11, 2020

here maybe we should add it, thanks!

@thinkerou
Copy link
Member

@thinkerou thinkerou commented Dec 11, 2020

@appleboy please review the pull request, thanks!

@appleboy
Copy link
Member

@appleboy appleboy commented Dec 11, 2020

@mask-pp Please provide the benchmark report?

@mask-pp
Copy link
Author

@mask-pp mask-pp commented Dec 11, 2020

@mask-pp Please provide the benchmark report?

ok, Is about StringToBytes change or others?

@appleboy
Copy link
Member

@appleboy appleboy commented Dec 11, 2020

@thinkerou
Copy link
Member

@thinkerou thinkerou commented Dec 23, 2020

any update? Thanks

@mask-pp
Copy link
Author

@mask-pp mask-pp commented Dec 23, 2020

any update? Thanks

already updated bentchmark report, please check. thanks!

@appleboy
Copy link
Member

@appleboy appleboy commented Dec 23, 2020

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

@mask-pp
Copy link
Author

@mask-pp mask-pp commented Dec 23, 2020

@mask-pp mask-pp closed this Dec 23, 2020
@thinkerou
Copy link
Member

@thinkerou thinkerou commented Dec 29, 2020

why closed?

appleboy pushed a commit that referenced this issue Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants