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

Optimize TimeUtil get timestamp logic to optimize performance for Sentinel 1.8.x #2909

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icodening
Copy link
Contributor

Describe what this PR does / why we need it

优化获取时间戳逻辑

Does this pull request fix one issue?

Fixes #2902

Describe how you did it

简化读/写计数器

在频繁获取时间戳的场景中,或许并不需要过于精确的计数器。当前版本的读写计数器是精确累加开销稍大。

@icodening
Copy link
Contributor Author

修改计数方式前:
Benchmark                                           (length)   Mode  Cnt       Score       Error  Units
SentinelEntryBenchmark.testSingleThreadSingleEntry        25  thrpt   25  502359.903 ± 10386.042  ops/s
修改计数方式后:
Benchmark                                           (length)   Mode  Cnt       Score      Error  Units
SentinelEntryBenchmark.testSingleThreadSingleEntry        25  thrpt   25  622161.827 ± 9694.449  ops/s

@yanxianchao
Copy link

感觉有两个问题
1、调用量的统计周期不是一个确定的周期。
2、reads、writes变量不需要保证原子性吗?即使不需要原子性,可见性也不需要吗?

@icodening
Copy link
Contributor Author

感觉有两个问题
1、调用量的统计周期不是一个确定的周期。
2、reads、writes变量不需要保证原子性吗?即使不需要原子性,可见性也不需要吗?

  1. idle周期300ms,繁忙时1ms. 虽然不是一个确定的周期,但我理解我只需要上一次check时的read / write 快照值即可,并不需要一个精确值来判断。当然这里还有改进空间,比如check守护线程动态调整阈值:当RUNNING时间隔为1ms如果使用原本的阈值会过高。
  2. 原子性我认为是可以不用保证的,主要是一个模糊值,该场景下过分考虑精确值或许意义不大。至于可见性是需要保证的,但守护线程已经周期性的sleep释放cpu了,守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性

@yanxianchao
Copy link

感觉有两个问题
1、调用量的统计周期不是一个确定的周期。
2、reads、writes变量不需要保证原子性吗?即使不需要原子性,可见性也不需要吗?

  1. idle周期300ms,繁忙时1ms. 虽然不是一个确定的周期,但我理解我只需要上一次check时的read / write 快照值即可,并不需要一个精确值来判断。当然这里还有改进空间,比如check守护线程动态调整阈值:当RUNNING时间隔为1ms如果使用原本的阈值会过高。
  2. 原子性我认为是可以不用保证的,主要是一个模糊值,该场景下过分考虑精确值或许意义不大。至于可见性是需要保证的,但守护线程已经周期性的sleep释放cpu了,守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性

不加volatile是有问题的,根据java内存模型的规定,如果没有volatile,各种jvm的实现可以非常宽松的去实现对共享变量的写入,读取线程可能永远读不到最新的值,而不是“守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性”

@icodening
Copy link
Contributor Author

感觉有两个问题

1、调用量的统计周期不是一个确定的周期。

2、reads、writes变量不需要保证原子性吗?即使不需要原子性,可见性也不需要吗?

  1. idle周期300ms,繁忙时1ms. 虽然不是一个确定的周期,但我理解我只需要上一次check时的read / write 快照值即可,并不需要一个精确值来判断。当然这里还有改进空间,比如check守护线程动态调整阈值:当RUNNING时间隔为1ms如果使用原本的阈值会过高。
  1. 原子性我认为是可以不用保证的,主要是一个模糊值,该场景下过分考虑精确值或许意义不大。至于可见性是需要保证的,但守护线程已经周期性的sleep释放cpu了,守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性

不加volatile是有问题的,根据java内存模型的规定,如果没有volatile,各种jvm的实现可以非常宽松的去实现对共享变量的写入,读取线程可能永远读不到最新的值,而不是“守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性”

目前测试hotspot下是可行的,你那边可测试其他类型的jvm么?

@sczyh30 sczyh30 added to-review To review area/performance Issues or PRs related to runtime performance kind/enhancement Category issues or prs related to enhancement. labels Oct 14, 2022
@yanxianchao
Copy link

感觉有两个问题

1、调用量的统计周期不是一个确定的周期。

2、reads、writes变量不需要保证原子性吗?即使不需要原子性,可见性也不需要吗?

  1. idle周期300ms,繁忙时1ms. 虽然不是一个确定的周期,但我理解我只需要上一次check时的read / write 快照值即可,并不需要一个精确值来判断。当然这里还有改进空间,比如check守护线程动态调整阈值:当RUNNING时间隔为1ms如果使用原本的阈值会过高。
  1. 原子性我认为是可以不用保证的,主要是一个模糊值,该场景下过分考虑精确值或许意义不大。至于可见性是需要保证的,但守护线程已经周期性的sleep释放cpu了,守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性

不加volatile是有问题的,根据java内存模型的规定,如果没有volatile,各种jvm的实现可以非常宽松的去实现对共享变量的写入,读取线程可能永远读不到最新的值,而不是“守护线程可在下个周期读取到新值,我们不需要再额外加volatile来保证更实时的可见性”

目前测试hotspot下是可行的,你那边可测试其他类型的jvm么?

不用测试,按照java内存模型的约定写代码就行,可以看下JSR133或者《java concurrency in practice》相关章节

@icodening
Copy link
Contributor Author

嗯嗯,加上volatile来保证当然是可行的。但我在想既然sleep也可以在不确定的时机中刷新,且此时需要的是一个模糊值,能否省略掉volatile

@sczyh30 sczyh30 changed the title optimize get timestamp Optimize TimeUtil get timestamp logic to optimize performance for Sentinel 1.8.x Jan 11, 2023
@sczyh30
Copy link
Member

sczyh30 commented Jan 11, 2023

cc @jasonjoo2010 for review

private static TimeUtil INSTANCE;

private volatile long currentTimeMillis;
private volatile STATE state = STATE.IDLE;

private LeapArray<Statistic> statistics;
private long reads = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe volatile is needed here (to ensure semantics).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Issues or PRs related to runtime performance kind/enhancement Category issues or prs related to enhancement. to-review To review
Projects
None yet
3 participants