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

[dlog] use Atomic***FieldUpdater and LongAdder if possible #1299

Closed
wants to merge 2 commits into from

Conversation

sijie
Copy link
Member

@sijie sijie commented Mar 28, 2018

Descriptions of the changes in this PR:

AtomicFieldUpdater + volatile provides similar guarantees as Atomic but use much fewer memory.
In dlog, when handling large number of streams, there can be a lot of Atomic fields with LogHandler, SegmentWriter and such.
Switching using Atomic to using AtomicFieldUpdater + volatile will save a lot of memory. See details

LongAdder is less contended across threads, which is more preferable to AtomicLong when multiple threads update a common sum. E.g. SampledMovingAverageRate.
Switching to use LongAdder if AtomicLong is not needed. See details

Master Issue: #


Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

  • Make sure the PR title is formatted like:
    <BP-#>: Description of bookkeeper proposal
    e.g. BP-1: 64 bits ledger is support
  • Attach the master issue link in the description of this PR.
  • Attach the google doc link if the BP is written in Google Doc.

Otherwise:

  • Make sure the PR title is formatted like:
    <Issue #>: Description of pull request
    e.g. Issue 123: Description ...
  • Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.
  • Replace <Issue #> in the title with the actual Issue number.

Atomic**FieldUpdater + volatile provides similar guarantees as Atomic*** but use much fewer memory.
In dlog, when handling large number of streams, there can be a lot of Atomic*** fields with `LogHandler`, `SegmentWriter` and such.
Switching using Atomic*** to using Atomic**FieldUpdater + volatile will save a lot of memory. See [details](http://normanmaurer.me/blog/2013/10/28/Lesser-known-concurrent-classes-Part-1/)

LongAdder is less contended across threads, which is more preferable to AtomicLong when multiple threads update a common sum. E.g. SampledMovingAverageRate.
Switching to use LongAdder if AtomicLong is not needed. See [details](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/atomic/LongAdder.html)
@sijie
Copy link
Member Author

sijie commented Mar 28, 2018

/cc @philipsu522 for reviews

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1 the change looks good
I did mostly a static analysis of the change.

@philipsu522
Copy link

LGTM!

Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

+1,
@sijie, There seems also some AtomicInt AtomicLong in bk code, Is there a plan to replace them?

@sijie
Copy link
Member Author

sijie commented Mar 29, 2018

@jiazhai - Change to AtomicFieldUpdater only help a lot when you have a lot of these AtomicInt and AtomicLong. The places I am changing in dlog is like segment / handler which is per stream. If you have a lot of streams, changing it to AtomicFieldUpdater can save a lot of spaces. Otherwise it is okay to just use AtomicInt or AtomicLong

@sijie sijie added this to the 4.7.0 milestone Mar 29, 2018
Copy link

@yzang yzang left a comment

Choose a reason for hiding this comment

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

LGTM+1

@sijie sijie closed this in db713f3 Mar 29, 2018
@sijie sijie deleted the use_atomic_field_updater branch July 16, 2018 02:54
@sijie sijie mentioned this pull request Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants