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

syncx: 支持分key加锁 #225

Merged
merged 8 commits into from
Oct 13, 2023
Merged

syncx: 支持分key加锁 #225

merged 8 commits into from
Oct 13, 2023

Conversation

WeiJiadong
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Oct 8, 2023

Codecov Report

Merging #225 (7398862) into dev (afcc565) will increase coverage by 0.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #225      +/-   ##
==========================================
+ Coverage   95.73%   95.93%   +0.19%     
==========================================
  Files          57       58       +1     
  Lines        3094     3122      +28     
==========================================
+ Hits         2962     2995      +33     
+ Misses        101       99       -2     
+ Partials       31       28       -3     
Files Coverage Δ
syncx/segment_key_lock.go 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@WeiJiadong WeiJiadong changed the title syncx: 支持分key加锁#224 syncx: 支持分key加锁 Oct 8, 2023
@WeiJiadong WeiJiadong mentioned this pull request Oct 8, 2023
@junwense
Copy link

junwense commented Oct 8, 2023

hash冲突了能解决么

@flycash
Copy link
Contributor

flycash commented Oct 8, 2023

哈希冲突了无所谓,也就是多个 key 共用一个锁而已。但是相比全局一个锁,并发的竞争激烈程度是下降了的

// RLock 读锁加锁
func (s *SegmentKeysLock) RLock(key string) {
hash := s.hash(key)
lock := &s.locks[hash%uint32(len(s.locks))]
Copy link
Contributor

Choose a reason for hiding this comment

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

我的理解是不用取地址,本身不会触发复制吧?

Copy link
Contributor Author

@WeiJiadong WeiJiadong Oct 8, 2023

Choose a reason for hiding this comment

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

主要有提示,比如go vet,不想破坏锁不能复制的语义。
其次,这里如果不取地址的话,相当于每次拷贝出一个新的锁对象。

Copy link
Contributor

Choose a reason for hiding this comment

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

如果这样的话,直接维持住指针。而后在 New 的时候就直接搞出来指针。相当于把这里取地址的性能开销,挪过去了最开始的初始化的时候。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已改

@WeiJiadong
Copy link
Contributor Author

hash冲突了能解决么

不能,但冲突也不影响。现在尽量做的是让它更均匀,尽量避免冲突,冲突时,lock会阻塞。

syncx/segment_key_lock_test.go Show resolved Hide resolved
syncx/segment_key_lock_test.go Outdated Show resolved Hide resolved
syncx/segment_key_lock_test.go Outdated Show resolved Hide resolved
syncx/segment_key_lock_test.go Show resolved Hide resolved
@junwense
Copy link

junwense commented Oct 9, 2023

哈希冲突了无所谓,也就是多个 key 共用一个锁而已。但是相比全局一个锁,并发的竞争激烈程度是下降了的

如果锁的数量不多,并且业务允许,这个确实挺好的,这样也不用考虑锁回收和并发的问题,我开始想的是把锁的获取封装起来,对锁的获取提供不同的支持,比如hash是否允许冲突,以及底层数据结构的实现,不过这样并发问题又得考虑,这样又复杂了

Comment on lines 30 to 31
var writeDone int32
var readStarted int32
Copy link
Collaborator

Choose a reason for hiding this comment

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

go 1.21中有atomic.Bool可以看一下,语义更准确

Copy link
Collaborator

@longyue0521 longyue0521 Oct 9, 2023

Choose a reason for hiding this comment

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

另外这个测试用例有些复杂且意图有些不清晰, 是不是可以这样:

  1. hash值相同的key,验证并发读写、读读、写写,只要验证有竞争即可
// 核心伪代码
// 真实竞争数据
data = ""

// 启动多个个写协程
for ... {
go func () {
      s.Lock(Key)
      data = "1"
      s.UnLock(Key)
}
}
// 启动1个读协程, 多个写与一个读竞争大概率写赢,读到写后的新数据
go func () {
      s.RLock(Key)
      ch <-data
      s.RUnLock(Key)
}

assert.Equal(t, "1", <-ch)
  1. hash值不同的key, 相互竞争不受控制, 这个不好写可以不写
// 真实竞争数据
data= ""


// 对key1数据写
go func () {
      s.Lock(Key1)
      data = "1"
      s.UnLock(Key1)
}
// 对key2数据写
go func () {
      s.Lock(Key2)
      data = "2"
      s.UnLock(Key2)
}
// 检测到竞争

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go 1.21中有atomic.Bool可以看一下,语义更准确
这里也可以写别的值,主要是验证是否写了后能被读到。
另外,如果用atomic.Bool这个特性,会对使用者版本有要求,要升级才能支持,不然编译不过。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

另外这个测试用例有些复杂且意图有些不清晰, 是不是可以这样:

  1. hash值相同的key,验证并发读写、读读、写写,只要验证有竞争即可
// 核心伪代码
// 真实竞争数据
data = ""

// 启动多个个写协程
for ... {
go func () {
      s.Lock(Key)
      data = "1"
      s.UnLock(Key)
}
}
// 启动1个读协程, 多个写与一个读竞争大概率写赢,读到写后的新数据
go func () {
      s.RLock(Key)
      ch <-data
      s.RUnLock(Key)
}

assert.Equal(t, "1", <-ch)
  1. hash值不同的key, 相互竞争不受控制, 这个不好写可以不写
// 真实竞争数据
data= ""


// 对key1数据写
go func () {
      s.Lock(Key1)
      data = "1"
      s.UnLock(Key1)
}
// 对key2数据写
go func () {
      s.Lock(Key2)
      data = "2"
      s.UnLock(Key2)
}
// 检测到竞争

确实,理论上是应该这么验证,不仅需要覆盖代码,还需要验证读写锁的特性。
我补一下关于读写锁特性的验证。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

思考了下,似乎不借助time.Sleep之类的,写写互斥的测试用例不好写,会卡死测试进程。

Copy link
Collaborator

Choose a reason for hiding this comment

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

其实最后那个s.Lock(Key1)与 s.Lock(Key2),是想用一个反例来验证底层,对于两个hash值不同的key使用的是两把读写锁,而不是同一把锁, 验证的方式就是运行测试的时候加-race参数会报错 —— data还是被两个写, 或读写竞争访问了, 但这样的“测试”没法编写.所以只能通过review来确保,不同hash值的key使用的是两把读写锁.

Copy link
Collaborator

Choose a reason for hiding this comment

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

go 1.21中有atomic.Bool可以看一下,语义更准确
这里也可以写别的值,主要是验证是否写了后能被读到。
另外,如果用atomic.Bool这个特性,会对使用者版本有要求,要升级才能支持,不然编译不过。

我写错了 go1.19中就支持了atomic.Bool, ekit要求1.20完全可以用

Copy link
Contributor

Choose a reason for hiding this comment

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

@longyue0521 确认没问题你就直接合并了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

@flycash 好的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go 1.21中有atomic.Bool可以看一下,语义更准确
这里也可以写别的值,主要是验证是否写了后能被读到。
另外,如果用atomic.Bool这个特性,会对使用者版本有要求,要升级才能支持,不然编译不过。

我写错了 go1.19中就支持了atomic.Bool, ekit要求1.20完全可以用

已修改成atomic.Bool,以及补齐了用例注释说明

@longyue0521 longyue0521 linked an issue Oct 9, 2023 that may be closed by this pull request
Copy link
Contributor

@flycash flycash left a comment

Choose a reason for hiding this comment

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

我先合并了。这里面确实不好测试,我引入 TryXXX 的 API 来测试

@flycash flycash merged commit e2bf6f7 into ecodeclub:dev Oct 13, 2023
6 checks passed
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.

syncx: SegmentKeysLock
4 participants