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

Impl: reduce syscall and memcopy for multiple package #36

Merged
merged 3 commits into from
Apr 8, 2020

Conversation

watermelo
Copy link
Member

@watermelo watermelo commented Apr 2, 2020

origin issue AlexStocks#33

@auto-comment
Copy link

auto-comment bot commented Apr 2, 2020

Thank your for raising your pull request.
Please make sure you have followed our contributing guidelines. We will review it as soon as possible

@codecov-io
Copy link

codecov-io commented Apr 2, 2020

Codecov Report

Merging #36 into master will increase coverage by 0.61%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   69.61%   70.22%   +0.61%     
==========================================
  Files           7        7              
  Lines        1415     1404      -11     
==========================================
+ Hits          985      986       +1     
+ Misses        336      328       -8     
+ Partials       94       90       -4     
Impacted Files Coverage Δ
session.go 65.48% <83.33%> (+0.73%) ⬆️
connection.go 79.53% <93.75%> (+0.96%) ⬆️

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 52cde59...8b02420. Read the comment docs.

@gaoxinge
Copy link

gaoxinge commented Apr 2, 2020

LGTM

@wongoo
Copy link

wongoo commented Apr 2, 2020

net.Buffers() is a great implement

session.go Outdated Show resolved Hide resolved
connection.go Outdated
return length, perrors.WithStack(err)
}

return 0, perrors.Errorf("illegal @pkg{%#v} type", pkg)
//return length, err

Choose a reason for hiding this comment

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

delete unuseful line ?

@gaoxinge
Copy link

gaoxinge commented Apr 3, 2020

@watermelo Can WriteBytes and WriteBytesArray in session.go be unified to one function? They look very similar.

@AlexStocks
Copy link

AlexStocks commented Apr 3, 2020

@watermelo Can WriteBytes and WriteBytesArray in session.go be unified to one function? They look very similar.

直接中文回复你吧。从功能上讲,你的理解当然是对的,但是基于这两个函数都是 public 的,所以基于最小改动原则,个人建议不合并。

@gaoxinge
Copy link

gaoxinge commented Apr 4, 2020

@AlexStocks @watermelo 嗯,是的。还有一个小小的问题,就是session.go里面的incWritePkgNum()

https://github.com/dubbogo/getty/blob/8b02420a4bc8f7928b22ccf6fb1caaecab5b2e8e/session.go#L431-L439

是不是需要放到下面的用connection.go里面?

https://github.com/dubbogo/getty/blob/8b02420a4bc8f7928b22ccf6fb1caaecab5b2e8e/connection.go#L292-L299

这样看起来是不是统一点?

@watermelo
Copy link
Member Author

@AlexStocks @watermelo 嗯,是的。还有一个小小的问题,就是session.go里面的incWritePkgNum()

https://github.com/dubbogo/getty/blob/8b02420a4bc8f7928b22ccf6fb1caaecab5b2e8e/session.go#L431-L439

是不是需要放到下面的用connection.go里面?

https://github.com/dubbogo/getty/blob/8b02420a4bc8f7928b22ccf6fb1caaecab5b2e8e/connection.go#L292-L299

这样看起来是不是统一点?

@gaoxinge done

@gaoxinge
Copy link

gaoxinge commented Apr 7, 2020

@watermelo Great job! LGTM.

Copy link

@fangyincheng fangyincheng left a comment

Choose a reason for hiding this comment

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

LGTM

@AlexStocks AlexStocks merged commit 45d3d7d into apache:master Apr 8, 2020
yqxu pushed a commit to yqxu/dubbo-getty that referenced this pull request Oct 15, 2021
Mod: remove useless return and make runUDPEventLoop run in goroutine
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.

6 participants