Use CAS for safer counter collection #394

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@isaacl
isaacl commented Sep 20, 2016

Note: this patch only fixes the scala counter. Other Kamon metrics have similar semantics which could benefit from CAS.

For #393

@@ -182,6 +186,13 @@ final boolean casBase(long cmp, long val) {
}
/**
+ * CASes the base field.
@dpsoft
dpsoft Sep 28, 2016 edited Contributor

@isaacl This code are using Java 8 Unsafe intrinsics (in this case lock xchg) and isn't supported in prior java versions.
Maybe we could check if support getAndSetObject in this way Unsafe.class.getMethod("getAndSetObject", Object.class, Long.TYPE,Object.class) and use this method, otherwise fallback to something like:

 final long getAndSetBase(long val) {
  long v;
     do {
          v = UNSAFE.getLongVolatile(this, baseOffset);
      } while (!UNSAFE.compareAndSwapLong(this, baseOffset, v, val));
  return v;
 }

or simply use the code above (CAS version, I do not know if the code compiles ;)) and the same for the final long getAndSet(long val)

@isaacl
isaacl Sep 28, 2016

If getAndSetLong was added too recently I'd suggest adding the function as a static helper, as it's needed for both the base get and the cell get.

The code you wrote is identical to the actual impl.

@dpsoft
dpsoft Sep 28, 2016 Contributor

Yes, is a good idea!

@isaacl
isaacl commented Sep 28, 2016

mostly sent this pull-req as a request for comment, as I was surprised to see this type of issue in Kamon. But if you're willing to review I will invest time to clean up this pull req etc.

@dpsoft
Contributor
dpsoft commented Sep 28, 2016 edited

@isaacl thanks for find this issue, no problem, we can follow the issue in our side (It was a surprise for us also, this is embarrassing) if you want?

@ivantopo
Contributor

hey @isaacl, it would be awesome if you can put that extra time to clean up and get this merged! :).. I'm sorry that we didn't get back to you right away, we certainly did take a look at this as soon as you proposed it because at this point, we shouldn't have this kind of bugs. I would like to move this forward as soon as possible and publish a new release with it. Thanks in advance for your effort! And, if time doesn't favor you let us know and we will take it from here, regards!

@isaacl
isaacl commented Sep 28, 2016

I should have some time in the next couple days, will follow up when the patch is cleaned up.

@dpsoft dpsoft added this to the 0.6.3 milestone Sep 30, 2016
@dpsoft
Contributor
dpsoft commented Oct 4, 2016

related to #399

@dpsoft dpsoft closed this Oct 4, 2016
@isaacl
isaacl commented Oct 5, 2016

Thanks for handling. Cheers guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment