-
Notifications
You must be signed in to change notification settings - Fork 99
Parameterize memory size and some key improvements #153
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
Conversation
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #153 +/- ##
==========================================
- Coverage 54.11% 54.00% -0.11%
==========================================
Files 89 89
Lines 7904 7997 +93
==========================================
+ Hits 4277 4319 +42
- Misses 3233 3275 +42
- Partials 394 403 +9
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make default values constants or variables and add comments for them rather than directly use them everywhere.
Instead of 2<<20
or 8<<20
, I prefer 1<<21
or 1<<24
which seems to be clearer.
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Not quite follow this argument. I remember I'm not using this toolkit. |
Make sense, I will tweak them.
|
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Fixes apache/skywalking#9383 |
}(value) | ||
} | ||
|
||
func (s *Writer) Close() error { | ||
return s.ch.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we removed Close()
here, is it possible we still have many goroutines waiting to write to the channel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use close to notify the receivers, not the senders. The senders will be waiting to write. That is the purpose of this process. They won't block the GC from collecting them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use close to notify the receivers, not the senders. The senders will be waiting to write. That is the purpose of this process.
Yes, we can use some stop channel to notify the receivers, but what about the senders?
They won't block the GC from collecting them.
From this issue, golang/go#19702, Go may not be clever enough now to garbage collect blocking goroutines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Automatically closing always has this kind of bugs in GC phases.
Be careful. We don't want a leak 😄
verification round Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
Signed-off-by: Gao Hongtao <hanahmily@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We can create an issue to track goroutine leak issue later.
These changes introduce several parameters to control memory usage. I assigned some defaults to them, and now the initial memory is around 2GB 🎉
There are a series of improvements:
run.Chan
, which could cause the data race. We don't have to close a channel more than notifying the receivers. @lujiajing1126 Please don't try to use it.measure
package.Signed-off-by: Gao Hongtao hanahmily@gmail.com