*: use AtomicU64 in value #64

overvenus · 2016-09-19T07:24:16Z

Atomic value:

test counter::bench_counter_no_labels                   ... bench:          14 ns/iter (+/- 0)
test counter::bench_counter_with_label_values           ... bench:          98 ns/iter (+/- 7)
test counter::bench_counter_with_mapped_labels          ... bench:         367 ns/iter (+/- 53)
test counter::bench_counter_with_prepared_mapped_labels ... bench:         213 ns/iter (+/- 18)
test gauge::bench_gauge_no_labels                       ... bench:          14 ns/iter (+/- 0)
test gauge::bench_gauge_with_label_values               ... bench:          94 ns/iter (+/- 7)

Mutex value:

test counter::bench_counter_no_labels                   ... bench:          44 ns/iter (+/- 2)
test counter::bench_counter_with_label_values           ... bench:         136 ns/iter (+/- 4)
test counter::bench_counter_with_mapped_labels          ... bench:         400 ns/iter (+/- 44)
test counter::bench_counter_with_prepared_mapped_labels ... bench:         250 ns/iter (+/- 4)
test gauge::bench_gauge_no_labels                       ... bench:          43 ns/iter (+/- 1)
test gauge::bench_gauge_with_label_values               ... bench:         131 ns/iter (+/- 7)

cc #59

siddontang · 2016-09-19T07:27:12Z

I prefer extracting these to a AtomicFloat structure.

siddontang · 2016-09-19T11:35:58Z

.travis.yml

-  - cargo test $FEATURE
+    script:
+      - cargo test --features dev
+      - cargo test --features="dev nightly"


why two cargo test?

I want to test nigltly feature too.

so why not test "dev nightly" directly?

The default feature uses Mutex<f64>, the nightly uses AtomicF64.

siddontang · 2016-09-19T11:39:27Z

src/lib.rs

@@ -41,6 +42,8 @@ mod registry;
 mod vec;
 mod histogram;
 mod push;
+#[cfg(feature="nightly")]
+mod atomicf64;


where is atomicf64?

Oops, my fault, now it's here, please review.

siddontang · 2016-09-19T11:45:59Z

src/value.rs

    pub val_type: ValueType,
    pub label_pairs: Vec<LabelPair>,
+
+    #[cfg(not(feature = "nightly"))]
+    pub val: RwLock<f64>,


wrap all in AtomicF64, not in value.
Histogram will use AtomicF64 too. It is not good to use RwLock and AtomicF64 in two places and we have too many nightly cfg flags.

siddontang · 2016-09-19T12:31:54Z

Histogram can use AtomicF64 too.

BusyJay · 2016-09-19T12:29:27Z

src/atomicf64.rs

+    pub fn inc_by(&self, delta: f64) {
+        loop {
+            let current = self.inner.load(Ordering::Acquire);
+            let new = current.as_f64() + delta;


Why not just use std::mem::transmute?

BusyJay · 2016-09-19T12:30:51Z

src/atomicf64.rs

+
+    #[cfg(feature = "nightly")]
+    #[test]
+    fn test_atomic_atomicf64() {


I think it's ok to use the same test case no matter features is enable or not.

BusyJay · 2016-09-19T12:33:36Z

src/atomicf64.rs

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#[cfg(not(feature = "nightly"))]


Why not put these into a submodule and then reexport it like:

#[cfg(feature = "nightly")] mod imp { ... } #[cfg(not(feature = "nightly"))] mod imp { ... } pub use imp::AtomicF64;

overvenus · 2016-09-19T13:00:03Z

@siddontang
I will send a PR for histogram separately.

siddontang · 2016-09-19T13:08:33Z

LGTM

PTAL @BusyJay @disksing

siddontang · 2016-09-19T13:09:38Z

It is ok to use AtomicF64 in histogram here.

ngaut · 2016-09-19T13:21:24Z

src/atomicf64.rs

+
+        #[inline]
+        pub fn get(&self) -> f64 {
+            self.inner.load(Ordering::Acquire).as_f64()


Ordering::Relex is enough

BusyJay

rest LGTM

BusyJay · 2016-09-19T13:40:58Z

src/atomicf64.rs

+
+        #[inline]
+        pub fn set(&self, val: f64) {
+            *(self.inner).write().unwrap() = val;


Why parentheses?

Better readability.

I don't think so. It make no sense to put self.inner into parentheses. It should be either *(self.inner.write().unwrap()) = val or *self.inner.write().unwrap() = val.

BusyJay · 2016-09-19T13:42:29Z

src/atomicf64.rs

+        }
+    }
+
+    trait Transform64bits: Copy {


Why define a trait? Isn't it enough to just use the transmute function?

Too many unsafe code block, although they are the same. I think as_u64 and as_f64 are handy.

I think you have more lines of code than it needs to when just using transmute. If you don't want to unsafe everywhere, just define two wrapper functions should the work too.

BusyJay · 2016-09-19T14:24:29Z

LGTM

This builds on the work in tikv#64 to allow stable Rust users to get atomics if they run on 64-bit architectures.

This builds on the work in tikv#64 to allow stable Rust users to get atomics if they run on 64-bit architectures. Tested: Ran tests on the following combinations (all unknown-linux-gnu): - stable Rust x86_64 - stable Rust i686 - nightly Rust x86_64 - nightly Rust i686 - nightly Rust x86_64 with `--features=nightly` - nightly Rust i686 with `--features=nightly`

This builds on the work in tikv#64 to allow stable Rust users to get atomics if they run on 64-bit architectures. Benchmark improvements: name old ns/iter new ns/iter diff ns/iter diff % counter::bench_counter_no_labels 49 16 -33 -67.35% counter::bench_counter_with_label_values 195 139 -56 -28.72% counter::bench_counter_with_mapped_labels 715 371 -344 -48.11% counter::bench_counter_with_prepared_mapped_labels 436 219 -217 -49.77% gauge::bench_gauge_no_labels 48 15 -33 -68.75% gauge::bench_gauge_with_label_values 167 115 -52 -31.14% histogram::bench_histogram_no_labels 179 30 -149 -83.24% histogram::bench_histogram_timer 276 138 -138 -50.00% histogram::bench_histogram_with_label_values 363 137 -226 -62.26% Benchmarks run with default features (no "nightly" feature) on nightly-2016-09-22, which corresponds roughly to 1.12.0. Tested: Ran tests on the following combinations (all unknown-linux-gnu): - stable Rust x86_64 - stable Rust i686 - nightly Rust x86_64 - nightly Rust i686 - nightly Rust x86_64 with `--features=nightly` - nightly Rust i686 with `--features=nightly`

*: use AtomicU64 in value

752c9ac

overvenus force-pushed the atomic-value branch from cba62e9 to 47dd784 Compare September 19, 2016 11:20

Address comments, update Makefile and update ci

bf3c39a

overvenus force-pushed the atomic-value branch from 47dd784 to bf3c39a Compare September 19, 2016 11:30

siddontang reviewed Sep 19, 2016

View reviewed changes

add missing mod atomicf64

09cde87

siddontang reviewed Sep 19, 2016

View reviewed changes

Address comments

362d1c2

BusyJay reviewed Sep 19, 2016

View reviewed changes

Address comments

8086d2e

ngaut reviewed Sep 19, 2016

View reviewed changes

src/atomicf64.rs

#[inline]

pub fn get(&self) -> f64 {

self.inner.load(Ordering::Acquire).as_f64()

Copy link

Member

ngaut Sep 19, 2016

Choose a reason for hiding this comment

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

Ordering::Relex is enough

Address comment

f857a0d

BusyJay reviewed Sep 19, 2016

View reviewed changes

overvenus added 3 commits September 19, 2016 21:55

fix ci

18d7a13

Address comments

8f4d3a7

Address comments

2ab0ca2

siddontang merged commit 500e8f5 into tikv:master Sep 19, 2016

overvenus mentioned this pull request Sep 19, 2016

Cargo: update prometheus tikv/tikv#1085

Merged

kamalmarhubi added a commit to kamalmarhubi/rust-prometheus that referenced this pull request Nov 4, 2016

atomic64: Use AtomicUsize on 64 bit targets

759ed5e

This builds on the work in tikv#64 to allow stable Rust users to get atomics if they run on 64-bit architectures.

kamalmarhubi added a commit to kamalmarhubi/rust-prometheus that referenced this pull request Nov 4, 2016

atomic64: Use AtomicUsize on 64 bit targets

c040869

This builds on the work in tikv#64 to allow stable Rust users to get atomics if they run on 64-bit architectures.

kamalmarhubi mentioned this pull request Nov 4, 2016

atomic64: Use AtomicUsize on 64-bit targets #82

Closed

overvenus deleted the atomic-value branch November 4, 2016 05:30

*: use AtomicU64 in value #64

*: use AtomicU64 in value #64

Uh oh!

Conversation

overvenus commented Sep 19, 2016

Uh oh!

siddontang commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddontang commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

overvenus commented Sep 19, 2016

Uh oh!

siddontang commented Sep 19, 2016

Uh oh!

siddontang commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay Sep 19, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay commented Sep 19, 2016

Uh oh!

Uh oh!

BusyJay Sep 19, 2016 •

edited

Loading