-
Notifications
You must be signed in to change notification settings - Fork 497
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
grpc filter use object pool #977
grpc filter use object pool #977
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #977 +/- ##
==========================================
+ Coverage 73.05% 73.33% +0.28%
==========================================
Files 134 136 +2
Lines 14866 15184 +318
==========================================
+ Hits 10860 11135 +275
- Misses 3380 3416 +36
- Partials 626 633 +7
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
hi @sodaRyCN , please checkout my comments, basically, I think this PR brings too much couplings to the code.
pkg/util/objectpool/objectpool.go
Outdated
if value, ok = m.pools.Load(key); !ok { | ||
m.lock.Lock() | ||
defer m.lock.Unlock() | ||
if value, _ = m.pools.Load(key); !ok { | ||
value = NewWithSpec(m.spec, ctx) | ||
defer m.pools.Store(key, value) | ||
} | ||
} |
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.
please use sync.Map.LoadOrStore
to refactor.
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.
I tried, but func LoadOrStore
means that a pool will be created every time, and when the initNum
of the pool's spec is non-zero, it will create some unused connections at the same time, then these connections need to be closed if it is load instead of store.
'LoadOrStore' cannot achieve an effect like this
func LoadOrStore(key interface{}, value func () interface{}) {
if key exists {
return old;
} else {
newValue = value()
store newValue
.....
}
}
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 do it like this, though there's still possibility that an unnecessary object being created, but it should be fine now.
if value, ok = m.pools.Load(key); !ok { | |
m.lock.Lock() | |
defer m.lock.Unlock() | |
if value, _ = m.pools.Load(key); !ok { | |
value = NewWithSpec(m.spec, ctx) | |
defer m.pools.Store(key, value) | |
} | |
} | |
if value, ok = m.pools.Load(key); !ok { | |
value = NewWithSpec(m.spec, ctx) | |
value, _ = m.pools.LoadOrStore(key, value) | |
} |
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.
In the case of concurrency, more than one Pool
woule be created by L191, and the initnum
of these pools may be 100、200, etc., and so many PoolObject
instances that will not be used will be created.
L192, the variable value
is reassigned, It is not good for the PoolObjects
that need to explicitly call destroy
or pool.Close
, such as the underlying connection, variables will be GC, but the connection still exists.
Based on the above reasons, I have a tried like the follow before deciding to use sync.Mutex
, but it have a problem:newValue.Close
will always be called.
if value, ok = m.pools.Load(key); !ok {
newValue = NewWithSpec(m.spec, ctx)
if value, ok = m.pools.LoadOrStore(key, newValue) ; ok{
newValue.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.
I checked the code, this has not been updated.
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.
As I said, using sync.Map
seems inappropriate in this case.
On the one hand it creates some unused object and needs to call Destroy
, on the other hand, as shown in the code example, LoadOrStore(key, value any) (actual any, loaded bool)
, loaded
always returns true, and newValue.Close
always be called. Of course, we could solve this bug like the following code (use object equals instead of loaded):
if value, ok = m.pools.Load(key); !ok {
newValue = NewWithSpec(m.spec, ctx)
if value, _= m.pools.LoadOrStore(key, newValue) ; value != newValue {
newValue.Close()
}
}
For the reasons, I think it would be better to use sync.Mutex
instead of LoadOrStore
.
bc44df0
to
14f130e
Compare
No description provided.