Added the PaddedAtomicLong by pveentjer · Pull Request #264 · JCTools/JCTools

pveentjer · 2019-09-13T04:17:31Z

A padded version of the java.util.atomic.AtomicLong.

franz1981 · 2019-09-13T07:07:04Z

Thanks for the contribution! Let me know when it can be reviewed :)

nitsanw

Thanks for the contribution. Please consider the comments. As mentioned this needs some consideration from a compatibility standpoint, or might need to be handling similar to MPSC linked queue CAS method.

nitsanw · 2019-09-13T09:41:07Z

+     * @see AtomicLong#getAndIncrement()
+     */
+    public final long getAndIncrement() {
+        return UNSAFE.getAndAddLong(this, VALUE_OFFSET, 1L);


This means no pre-8 compatibility, which is not necessarily bad, but needs consideration.

I had a look at the Java 6 code

public final long getAndAdd(long delta) { while (true) { long current = get(); long next = current + delta; if (compareAndSet(current, next)) return current; } }

So Unsafe.getAndSetLong has been replaced by something I expect to be a lot slower.

I made quick JMH benchmark:

@Fork(value = 2) @Warmup(iterations = 1) @BenchmarkMode(Mode.AverageTime) @OutputTimeUnit(TimeUnit.NANOSECONDS) @State(Scope.Benchmark) @Threads(4) public class GetAndAddBenchmark { public AtomicLong a; @Setup public void setup(){ a=new AtomicLong(); } @Benchmark public long slow() { long delta = 10; while (true) { long current = a.get(); long next = current + delta; if (a.compareAndSet(current, next)) return current; } } @Benchmark public long fast() { long delta = 10; return a.getAndAdd(delta); } }

And there is a significant performance loss with the slow approach:

Benchmark Mode Cnt Score Error Units GetAndAddBenchmark.fast avgt 10 118.111 ± 9.630 ns/op GetAndAddBenchmark.slow avgt 10 231.833 ± 30.669 ns/op

I had a peek at the generated Assembly (perfasm) and key difference is that the slow approach does a volatile read and then a 'lock cmpxchg' and the fast approach only needs a 'lock xadd'. The issue is that the former approach can run into a cas failure, while this can't happen with the lock xadd (hence latter approach is more performant).

In other words: there is a very significant performance loss if I would do it in the Java 6 way. Do you see any way to do it in a Java 6 compatible way but without the performance impact? One option would be to add a switch that chooses the appropriate logic based on the existence of Unsafe.getAndSetLong

@nitsanw could you have another look?

coveralls · 2019-09-16T01:40:10Z

Pull Request Test Coverage Report for Build 717

48 of 54 (88.89%) changed or added relevant lines in 1 file are covered.
30 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.4%) to 85.831%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
jctools-core/src/main/java/org/jctools/util/PaddedAtomicLong.java	48	54	88.89%

Files with Coverage Reduction	New Missed Lines	%
jctools-core/src/main/java/org/jctools/maps/NonBlockingSetInt.java	1	77.09%
jctools-core/src/main/java/org/jctools/queues/MpmcUnboundedXaddArrayQueue.java	2	96.06%
jctools-core/src/main/java/org/jctools/maps/NonBlockingHashMap.java	4	78.38%
jctools-core/src/main/java/org/jctools/maps/NonBlockingHashMapLong.java	6	78.64%
jctools-core/src/main/java/org/jctools/maps/NonBlockingIdentityHashMap.java	6	76.31%
jctools-core/src/main/java/org/jctools/maps/ConcurrentAutoTable.java	11	47.69%

Totals
Change from base Build 716:	-0.4%
Covered Lines:	4822
Relevant Lines:	5618

💛 - Coveralls

franz1981 · 2019-10-07T06:26:20Z

@pveentjer
Look at https://github.com/JCTools/JCTools/blob/master/jctools-core/src/main/java/org/jctools/queues/MpscLinkedQueue.java#L43

Probably for java 6 we don't have other options if not to accept it to be slower :)

nitsanw · 2019-11-04T19:22:15Z

@pveentjer I've pushed to your branch, please have a look, if it's all good please submit a unit test and rebase + squash commits to clean merge.
Sorry for the slow responses, and thanks for your contribution!

pveentjer · 2019-12-29T10:03:15Z

@nitsanw I'll get my act together today and finish up the work.

Once that is done I'll also add an PaddedAtomicReference.

pveentjer · 2019-12-29T11:33:33Z

I have added a unit tests and squashed the commits.

franz1981 · 2019-12-29T12:13:17Z

@pveentjer There is a build failure on the CI
A couple of notes:

getAndSet0 and getAndAdd0 could be copied directly on the callers, to save inlining budget
it would be nice to improve UnsafeAccess to allow forcing the cmpAndSwap versions of getAndSet, both for benchmarking purposes and to increase coverage while using jdk 8

nitsanw · 2019-12-30T15:53:42Z

[2019-12-29 11:42:41] [autobuild] [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project jctools-core: Compilation failure: Compilation failure: 
[2019-12-29 11:42:41] [autobuild] [ERROR] /opt/src/jctools-core/src/main/java/org/jctools/util/PaddedAtomicLong.java:[322,25] cannot find symbol
[2019-12-29 11:42:41] [autobuild] [ERROR]   symbol:   variable SUPPORTS_GET_AND_SET
[2019-12-29 11:42:41] [autobuild] [ERROR]   location: class org.jctools.util.UnsafeAccess
[2019-12-29 11:42:41] [autobuild] [ERROR] /opt/src/jctools-core/src/main/java/org/jctools/util/PaddedAtomicLong.java:[322,12] illegal start of type
[2019-12-29 11:42:41] [autobuild] [ERROR] /opt/src/jctools-core/src/main/java/org/jctools/util/PaddedAtomicLong.java:[340,25] cannot find symbol
[2019-12-29 11:42:41] [autobuild] [ERROR]   symbol:   variable SUPPORTS_GET_AND_SET

nitsanw · 2019-12-31T07:36:46Z

@pveentjer currently the build is failing, looks like you need to rebase on top of latest code and adjust. If you are quick we might be able to ship this with the 3.0 release this week.

pveentjer · 2020-10-16T12:28:10Z

Could someone have another look at this PR?

I'm not sure of copying the content of the methods is the right approach because it increases code duplication and it isn't trivial code.

nitsanw · 2020-10-19T15:56:24Z

+abstract class PaddedAtomicLongL1Pad extends Number implements java.io.Serializable {
+    private static final long serialVersionUID = 1;
+
+    transient long p01, p02, p03, p04, p05, p06, p07;


please use byte padding as per the rest of JCTools

Sure. Out of curiosity, is there any difference compared to using longs?

from JDK 15 the JVM started reordering fields to fill in gaps between object header and fields, resulting in long based padding fails.

nitsanw · 2020-11-26T07:22:31Z

The PR fits in with other work, I'm going to merge and work out the remaining bits. Thanks for the help @pveentjer !

nitsanw requested changes Sep 13, 2019

View reviewed changes

nitsanw force-pushed the PaddedAtomicLong branch from 0c7ebb0 to 3011dbf Compare November 4, 2019 19:20

pveentjer force-pushed the PaddedAtomicLong branch from 3011dbf to f17d6af Compare December 29, 2019 11:32

pveentjer force-pushed the PaddedAtomicLong branch from f17d6af to d49fc58 Compare December 29, 2019 11:36

pveentjer changed the title ~~WIP: Added the PaddedAtomicLong~~ Added the PaddedAtomicLong Dec 29, 2019

nitsanw force-pushed the master branch from 36d3fed to 88ca9a5 Compare May 22, 2020 15:53

pveentjer force-pushed the PaddedAtomicLong branch from d49fc58 to f7fade5 Compare October 5, 2020 07:45

Added the PaddedAtomicLong

b0c9557

pveentjer force-pushed the PaddedAtomicLong branch from f7fade5 to 87344cb Compare October 7, 2020 11:02

nitsanw reviewed Oct 19, 2020

View reviewed changes

WIP

abefafc

pveentjer force-pushed the PaddedAtomicLong branch from 87344cb to abefafc Compare October 21, 2020 12:07

nitsanw merged commit bd4ac47 into JCTools:master Nov 26, 2020

Conversation

pveentjer commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

franz1981 commented Sep 13, 2019

Uh oh!

nitsanw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nitsanw Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

pveentjer Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pveentjer Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coveralls commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 717

💛 - Coveralls

Uh oh!

franz1981 commented Oct 7, 2019

Uh oh!

nitsanw commented Nov 4, 2019

Uh oh!

pveentjer commented Dec 29, 2019

Uh oh!

pveentjer commented Dec 29, 2019

Uh oh!

franz1981 commented Dec 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nitsanw commented Dec 30, 2019

Uh oh!

nitsanw commented Dec 31, 2019

Uh oh!

pveentjer commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nitsanw Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

pveentjer Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitsanw Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

nitsanw commented Nov 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pveentjer commented Sep 13, 2019 •

edited

Loading

pveentjer Sep 20, 2019 •

edited

Loading

coveralls commented Sep 16, 2019 •

edited

Loading

franz1981 commented Dec 29, 2019 •

edited

Loading

pveentjer commented Oct 16, 2020 •

edited

Loading

pveentjer Oct 21, 2020 •

edited

Loading