feat: optimize no-op bitfield operations #849

Stebalien · 2022-09-08T04:47:29Z

We have quite a few bitfield operations where one bitfield is empty.
Optimize them.

codecov-commenter · 2022-09-08T04:52:32Z

Codecov Report

Merging #849 (62c9c9c) into master (9b1f206) will decrease coverage by 0.26%.
The diff coverage is 32.17%.

@@            Coverage Diff             @@
##           master     #849      +/-   ##
==========================================
- Coverage   51.29%   51.02%   -0.27%     
==========================================
  Files         123      124       +1     
  Lines       10060    10149      +89     
==========================================
+ Hits         5160     5179      +19     
- Misses       4900     4970      +70

Impacted Files	Coverage Δ
ipld/bitfield/src/ops.rs	`27.77% <27.77%> (ø)`
ipld/bitfield/src/lib.rs	`78.07% <100.00%> (+2.22%)`	⬆️
shared/src/version/mod.rs	`3.70% <0.00%> (-0.15%)`	⬇️

Stebalien · 2022-09-08T04:50:19Z

ipld/bitfield/src/lib.rs

    }
 }

 impl BitOrAssign<&BitField> for BitField {
    #[inline]
    fn bitor_assign(&mut self, rhs: &BitField) {
-        *self = &*self | rhs;
+        if !rhs.is_trivially_empty() {


saves a clone

Stebalien · 2022-09-08T04:50:30Z

ipld/bitfield/src/lib.rs

+    #[inline]
+    fn bitor_assign(&mut self, rhs: BitField) {
+        // by-value to optimize the case where one is empty.
+        let s = std::mem::take(self);


also may save a clone

I don't quite follow. The | op on 430 uses the bitor's defined above. Are you saying impl BitOr<&BitField> for &BitField { is less efficient than impl BitOr<BitField> for &BitField { apart from the clone? All I can see different is the clone.

That's the difference. But I'll see if I can simplify a bit.

Stebalien · 2022-09-08T04:50:45Z

ipld/bitfield/src/lib.rs

@@ -367,7 +406,20 @@ impl BitAnd<&BitField> for &BitField {

    #[inline]
    fn bitand(self, rhs: &BitField) -> Self::Output {
-        BitField::from_ranges(self.ranges().intersection(rhs.ranges()))
+        if self.is_trivially_empty() || rhs.is_trivially_empty() {
+            BitField::new()


This is free.

Stebalien · 2022-09-08T04:51:37Z

ipld/bitfield/src/lib.rs

+    }
+}
+
+impl BitAnd<BitField> for BitField {


Implementing by-value ops encourages users to perform by-value ops where possible (possibly leading to optimizations).

This uses impl BitAnd<&BitField> for &BitField { under the hood so there's no speedup to using this instead correct? But you're saying just letting them work with values directly can lead to user side optimizations?

Ah, no. I just don't want the user to try a by-value operation, notice that it doesn't work, then assume that bitfields don't support by-value operations.

Which is what I’ve been doing recently. 😂

We have quite a few bitfield operations where one bitfield is empty. Optimize them.

jennijuju · 2022-09-08T05:28:26Z

(are we supposed to backport this to release/v2? (I can totally do it just wanna confirm we should

Stebalien · 2022-09-08T05:34:12Z

(are we supposed to backport this to release/v2? (I can totally do it just wanna confirm we should

Ah.... no. This create isn't actually a part of the FVM, we just stashed it in this repo because we're LAAAAAAAAAAAAAAZY.

We can safely release this from master.

Stebalien · 2022-09-08T05:34:30Z

We've discussed breaking all the "ipld" creates into a new repo, but haven't because that's more work.

ZenGround0

I'm not so strong on rust's memory model so some of the tricks used here go over my head. However it looks correct.

ZenGround0 · 2022-09-09T21:55:07Z

ipld/bitfield/src/lib.rs

@@ -297,6 +297,12 @@ impl BitField {
                .all(|bit| self.unset.contains(&bit))
    }

+    /// Returns `true` if the bit field is _trivially_ empty. This is significantly faster than
+    /// checking if it's actually empty, but can be used to short-circuite certain operations.


Making sure I understand: to check if its actually empty we'd need to apply unset bits across set bits and ranges?

Yep. Basically, if it returns true, it's definitely empty. But if it returns false, we won't be able to know without doing a bit of work.

ZenGround0 · 2022-09-09T22:20:31Z

ipld/bitfield/src/lib.rs

+    #[inline]
+    fn bitor_assign(&mut self, rhs: BitField) {
+        // by-value to optimize the case where one is empty.
+        let s = std::mem::take(self);


I don't quite follow. The | op on 430 uses the bitor's defined above. Are you saying impl BitOr<&BitField> for &BitField { is less efficient than impl BitOr<BitField> for &BitField { apart from the clone? All I can see different is the clone.

ZenGround0 · 2022-09-09T22:22:35Z

ipld/bitfield/src/lib.rs

+
+    #[inline]
+    fn bitor(self, rhs: &BitField) -> Self::Output {
+        if rhs.is_trivially_empty() {


nit: unless I'm missing some speedup gain it would be nice to check in a consistent order. Other bitors do self, then rhs

I'm trying to avoid a clone if both are trivially empty. It probably won't matter in most case so I'll switch it back.

ZenGround0 · 2022-09-09T22:26:38Z

ipld/bitfield/src/lib.rs

+    }
+}
+
+impl BitAnd<BitField> for BitField {


This uses impl BitAnd<&BitField> for &BitField { under the hood so there's no speedup to using this instead correct? But you're saying just letting them work with values directly can lead to user side optimizations?

And put them in a new file. This is verbose, but should be easy to understand and reason about.

Stebalien · 2022-09-10T00:41:33Z

@ZenGround0 I broke the ops into a separate file and implemented them on all ref/non-ref combinations. It's very verbose, but should be easier to reason about.

Kubuxu · 2022-09-12T18:04:55Z

ipld/bitfield/src/ops.rs

+    #[inline]
+    fn bitxor(self, rhs: &BitField) -> Self::Output {
+        // Nothing to optimize.
+        BitField::from_ranges(self.ranges().symmetric_difference(rhs.ranges()))


Wouldn't XOR be a no-op if one of the sets is trivially empty?

Kubuxu

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: optimize no-op bitfield operations #849

feat: optimize no-op bitfield operations #849

Stebalien commented Sep 8, 2022

codecov-commenter commented Sep 8, 2022 •

edited

Loading

Stebalien Sep 8, 2022

Stebalien Sep 8, 2022

ZenGround0 Sep 9, 2022

Stebalien Sep 10, 2022

Stebalien Sep 8, 2022

Stebalien Sep 8, 2022

ZenGround0 Sep 9, 2022

Stebalien Sep 9, 2022

Kubuxu Sep 12, 2022

jennijuju commented Sep 8, 2022

Stebalien commented Sep 8, 2022

Stebalien commented Sep 8, 2022

ZenGround0 left a comment

ZenGround0 Sep 9, 2022

Stebalien Sep 9, 2022

ZenGround0 Sep 9, 2022

ZenGround0 Sep 9, 2022

Stebalien Sep 10, 2022

ZenGround0 Sep 9, 2022

Stebalien commented Sep 10, 2022

Kubuxu Sep 12, 2022

Kubuxu left a comment

feat: optimize no-op bitfield operations #849

feat: optimize no-op bitfield operations #849

Conversation

Stebalien commented Sep 8, 2022

codecov-commenter commented Sep 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennijuju commented Sep 8, 2022

Stebalien commented Sep 8, 2022

Stebalien commented Sep 8, 2022

ZenGround0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Sep 10, 2022

Choose a reason for hiding this comment

Kubuxu left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 8, 2022 •

edited

Loading