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

feat(freecache): v2 finish #702

Merged
merged 14 commits into from
Feb 17, 2023
Merged

Conversation

PengYechang
Copy link
Contributor

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@PengYechang PengYechang temporarily deployed to tablestore-live February 6, 2023 07:29 — with GitHub Actions Inactive
)

// DefaultConfig 返回默认配置 缓存容量256MB
func DefaultConfig() Config {
Copy link
Member

Choose a reason for hiding this comment

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

Config统一都用*Config

}

// SetName 设置缓存名称
func (c Config) SetName(name string) Config {
Copy link
Member

Choose a reason for hiding this comment

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

这些Set方法可以先去掉,变量已经export出来了,后面有必要再加呢?

}

type localStorage struct {
req Config
Copy link
Member

Choose a reason for hiding this comment

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

req->config

}

func Test_cache_GetAndSetCacheData(t *testing.T) {
oneCache := NewLocalCache[string, Student](DefaultConfig().
Copy link
Member

Choose a reason for hiding this comment

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

有没有可能把API设计为DefaultConfig().Build()? 和其他组件的API保持一致

@PengYechang PengYechang temporarily deployed to tablestore-live February 8, 2023 10:47 — with GitHub Actions Inactive
@PengYechang PengYechang temporarily deployed to tablestore-live February 8, 2023 10:48 — with GitHub Actions Inactive

// ParseSize parses a size string.
// Valid time units are "b" (or "byte"), "kb", "mb", "gb", "tb".
func ParseSize(s string) (Size, error) {
Copy link
Member

Choose a reason for hiding this comment

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

这个方法需要补充单测,或者考虑用其他的开源库也可以

Copy link
Member

Choose a reason for hiding this comment

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

func DefaultConfig(size Size) *Config {
once.Do(func() {
if size < 512*KB {
size = 256 * MB
Copy link
Member

Choose a reason for hiding this comment

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

也可以限制下最大内存

if size < 512*KB {
size = 256 * MB
}
innerCache = freecache.NewCache(int(size))
Copy link
Member

Choose a reason for hiding this comment

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

这个NewCache可以在解析完配置之后再做,DefaultConfig只设置默认配置

if val, ok := v[id]; ok {
cacheData = val
}
data, innerErr := json.Marshal(cacheData)
Copy link
Member

Choose a reason for hiding this comment

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

这个是否判断下,如果是proto.Message,就用proto来编解码

Copy link
Member

Choose a reason for hiding this comment

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

可以加个benchmark对比下proto和json两种对象的性能

@PengYechang PengYechang temporarily deployed to tablestore-live February 9, 2023 11:31 — with GitHub Actions Inactive
@PengYechang PengYechang temporarily deployed to tablestore-live February 9, 2023 12:37 — with GitHub Actions Inactive
@PengYechang PengYechang temporarily deployed to tablestore-live February 10, 2023 01:59 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #702 (18865c1) into master (57e23b6) will decrease coverage by 0.01%.
The diff coverage is n/a.

📣 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

@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
- Coverage   53.33%   53.33%   -0.01%     
==========================================
  Files         191      191              
  Lines       10545    10547       +2     
==========================================
+ Hits         5624     5625       +1     
  Misses       4498     4498              
- Partials      423      424       +1     
Impacted Files Coverage Δ
pkg/core/sentinel/config.go 65.71% <0.00%> (-4.29%) ⬇️
pkg/core/sentinel/datasouce_etcd.go 55.00% <0.00%> (-4.00%) ⬇️
pkg/server/xgrpc/server.go 74.07% <0.00%> (+0.48%) ⬆️
pkg/store/tablestore/instance.go 68.83% <0.00%> (+11.68%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@PengYechang PengYechang temporarily deployed to tablestore-live February 10, 2023 02:04 — with GitHub Actions Inactive
Copy link
Member

@sysulq sysulq left a comment

Choose a reason for hiding this comment

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

可以统一格式化下import的路径,然后我这边就没问题了


// id去重
ids = lo.Uniq(ids)
idsNone := make([]K, 0)
Copy link
Member

Choose a reason for hiding this comment

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

这里可以考虑预分配len(ids)的内存

cacheKey := c.getKey(key, id)
if resT, innerErr := c.GetCacheData(cacheKey); innerErr == nil && resT != nil {
var value V
if msg, ok := any(value).(proto.Message); ok { // Constrained to proto.Message
Copy link
Member

Choose a reason for hiding this comment

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

可以考虑封装下unmarshal和marshal的方法,参数是any

1、格式化import的路径
2、预分配len(ids)的内存
3、封装unmarshal和marshal方法
@PengYechang PengYechang temporarily deployed to tablestore-live February 14, 2023 02:54 — with GitHub Actions Inactive
miaofeng2027
miaofeng2027 previously approved these changes Feb 15, 2023
补充bench test
@PengYechang PengYechang temporarily deployed to tablestore-live February 15, 2023 02:50 — with GitHub Actions Inactive
add sync.Pool
@PengYechang PengYechang temporarily deployed to tablestore-live February 16, 2023 12:16 — with GitHub Actions Inactive
add sync.Pool
@PengYechang PengYechang temporarily deployed to tablestore-live February 16, 2023 12:25 — with GitHub Actions Inactive
change interface{} to any
@PengYechang PengYechang temporarily deployed to tablestore-live February 17, 2023 06:35 — with GitHub Actions Inactive
msgType := reflect.TypeOf(msg).Elem()

// Make a new one, and throw it back into T
msg = reflect.New(msgType).Interface().(proto.Message)
Copy link
Member

Choose a reason for hiding this comment

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

这里可以用sync.Pool缓存一下,对比看看

@sysulq sysulq merged commit 16b9788 into douyu:master Feb 17, 2023
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.

4 participants