Store `Feature` flags in line rather than on the heap by default (without increasing their size) #3730

TheBlueMatt · 2025-04-10T22:14:36Z

In a running LDK instance we generally have a ton of Features objects flying around, including ones per channel/peer in the NetworkGraph, ones per channel/peer in
Channel/ChannelManager, and ones inside of BOLT 11/12 Invoices.

Thus, its useful to avoid unecessary allocations in them to reduce heap fragmentation.

Luckily, Features generally don't have more than 15 bytes worth of flags, and because Vec has a NonNull pointer we can actually wrap a vec in a two-variant enum with zero additional space penalty.

While this patch leaves actually deserializing Features without allocating as a future exercise, immediately releasing the allocation is much better than holding it, and Features constructed through repeated set_* calls benefit from avoiding the Vec entirely.

ldk-reviews-bot · 2025-04-10T22:14:38Z

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

This avoids vec doubling while doing `Features` conversions.

lightning-types/src/features.rs

tnull · 2025-04-11T07:56:22Z

lightning-types/src/features.rs

+#[derive(Clone, PartialEq, Eq)]
+#[doc(hidden)]
+pub enum FeatureFlags {
+	Held { bytes: [u8; DIRECT_ALLOC_BYTES], len: u8 },


Should these be called OnStack/OnHeap or similar?

But its not necessarily on stack - its just in the object, and the object may be on heap or on stack.

Mhh. Good point, I just found the current naming a bit odd.

Maybe Heap and Direct? I agree its odd but I don't know what's better.

Maybe Heap and Direct?

Yeah, maybe? I guess if we're not conclusive about what's better, could also leave as is.

ldk-reviews-bot · 2025-04-14T00:00:54Z

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

fuzz/src/feature_flags.rs

valentinewallace · 2025-04-14T20:49:09Z

lightning-types/src/features.rs

+// Thus, as long as we never use more than 15 bytes for our Held variant `FeatureFlags` is the same
+// length as a `Vec` in memory.


Is it not 16 bytes (2 usizes as mentioned above) because we need a byte to write the type of features, or? Would be helpful to clarify that.

Right, 16 bytes minus the length byte, I'll clarify.

tnull · 2025-04-22T17:22:39Z

Seems CI is stuck here?

TheBlueMatt · 2025-04-24T16:35:07Z

Went ahead and squashed to see if CI will run again.

In a running LDK instance we generally have a ton of `Feature`s objects flying around, including ones per channel/peer in the `NetworkGraph`, ones per channel/peer in `Channel`/`ChannelManager`, and ones inside of BOLT 11/12 Invoices. Thus, its useful to avoid unecessary allocations in them to reduce heap fragmentation. Luckily, Features generally don't have more than 15 bytes worth of flags, and because `Vec` has a `NonNull` pointer we can actually wrap a vec in a two-variant enum with zero additional space penalty. While this patch leaves actually deserializing `Features` without allocating as a future exercise, immediately releasing the allocation is much better than holding it, and `Features` constructed through repeated `set_*` calls benefit from avoiding the Vec entirely.

TheBlueMatt · 2025-04-26T01:01:05Z

Also made clippy happy:

$ git diff-tree -U2 a07c020d4 4a841aac3diff --git a/lightning-types/src/features.rs b/lightning-types/src/features.rs
index 804083d65..d97347b47 100644
--- a/lightning-types/src/features.rs
+++ b/lightning-types/src/features.rs
@@ -816,5 +816,5 @@ impl DerefMut for FeatureFlags {
 impl PartialOrd for FeatureFlags {
 	fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
-		self.deref().partial_cmp(other.deref())
+		Some(self.cmp(other))
 	}
 }

valentinewallace · 2025-05-05T19:33:27Z

lightning-types/src/features.rs

+		}
+	}
+
+	fn resize(&mut self, new_len: usize, default: u8) {


It seems like we'd always want to resize with 0 in this context? Not sure this parameter is useful

It was just to keep the API identical to Vec's. I can drop it, but its also not adding complexity I think.

lightning-types/src/features.rs

valentinewallace · 2025-05-05T20:10:10Z

fuzz/src/feature_flags.rs

+	assert_eq!(v.len(), feat.len());
+	assert_eq!(v.deref(), feat.deref());
+	assert_eq!(old_v.deref_mut(), old_feat.deref_mut());


Why not have the same checks for the new and old sets of features?

We call this method once for each updated set of features. The old features here were already passed to this method as the new features in the previous loop iteration.

fuzz/src/feature_flags.rs

valentinewallace · 2025-05-05T20:14:12Z

fuzz/src/feature_flags.rs

+	if data.len() % 3 != 0 {
+		return;
+	}


I guess you could just redefine data to be a slice terminating 1 or 2 items early in the original slice, not a big deal though.

Yea, the fuzzer can deal :)

No new datastructure would be complete without a dedicated fuzzer, so we add one here.

codecov · 2025-05-05T21:50:23Z

Codecov Report

Attention: Patch coverage is 90.09901% with 10 lines in your changes missing coverage. Please review.

Project coverage is 90.99%. Comparing base (85185d8) to head (f491278).
Report is 98 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-types/src/features.rs	90.09%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3730      +/-   ##
==========================================
+ Coverage   89.11%   90.99%   +1.87%     
==========================================
  Files         156      158       +2     
  Lines      123514   138442   +14928     
  Branches   123514   138442   +14928     
==========================================
+ Hits       110070   125970   +15900     
+ Misses      10762     9853     -909     
+ Partials     2682     2619      -63

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ldk-reviews-bot requested a review from valentinewallace April 10, 2025 22:25

TheBlueMatt force-pushed the 2025-04-features-less-alloc branch 4 times, most recently from 61824a3 to 03aaa63 Compare April 10, 2025 23:54

Pre-allocate bytes in Features::to_context_internal

7fca90c

This avoids vec doubling while doing `Features` conversions.

TheBlueMatt force-pushed the 2025-04-features-less-alloc branch 2 times, most recently from 1160c31 to 2dd6c18 Compare April 11, 2025 00:31

TheBlueMatt mentioned this pull request Apr 11, 2025

ln: reduce size_of Event from 1680 B -> 576 B #3723

Open

tnull reviewed Apr 11, 2025

View reviewed changes

valentinewallace reviewed Apr 14, 2025

View reviewed changes

TheBlueMatt force-pushed the 2025-04-features-less-alloc branch from 2dd6c18 to fadb54f Compare April 15, 2025 13:12

TheBlueMatt force-pushed the 2025-04-features-less-alloc branch from fadb54f to a07c020 Compare April 24, 2025 16:34

TheBlueMatt force-pushed the 2025-04-features-less-alloc branch from a07c020 to 4a841aa Compare April 26, 2025 01:00

TheBlueMatt requested review from valentinewallace and tnull May 5, 2025 13:46

valentinewallace reviewed May 5, 2025

View reviewed changes

TheBlueMatt added 3 commits May 5, 2025 21:40

f test feature flags transitions

9362766

Fuzz the new FeatureFlags storage

d52760f

No new datastructure would be complete without a dedicated fuzzer, so we add one here.

f comments

f491278

TheBlueMatt force-pushed the 2025-04-features-less-alloc branch from 4a841aa to f491278 Compare May 5, 2025 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store `Feature` flags in line rather than on the heap by default (without increasing their size) #3730

Store `Feature` flags in line rather than on the heap by default (without increasing their size) #3730

TheBlueMatt commented Apr 10, 2025

ldk-reviews-bot commented Apr 10, 2025 •

edited

Loading

tnull Apr 11, 2025

TheBlueMatt Apr 11, 2025

tnull Apr 15, 2025

TheBlueMatt Apr 15, 2025

tnull Apr 17, 2025

ldk-reviews-bot commented Apr 14, 2025

valentinewallace Apr 14, 2025

TheBlueMatt Apr 15, 2025

tnull commented Apr 22, 2025

TheBlueMatt commented Apr 24, 2025

TheBlueMatt commented Apr 26, 2025

valentinewallace May 5, 2025

TheBlueMatt May 5, 2025

valentinewallace May 5, 2025

TheBlueMatt May 5, 2025

valentinewallace May 5, 2025

TheBlueMatt May 5, 2025

codecov bot commented May 5, 2025

		// Thus, as long as we never use more than 15 bytes for our Held variant `FeatureFlags` is the same
		// length as a `Vec` in memory.

Store Feature flags in line rather than on the heap by default (without increasing their size) #3730

Are you sure you want to change the base?

Store Feature flags in line rather than on the heap by default (without increasing their size) #3730

Conversation

TheBlueMatt commented Apr 10, 2025

ldk-reviews-bot commented Apr 10, 2025 • edited Loading

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

ldk-reviews-bot commented Apr 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnull commented Apr 22, 2025

TheBlueMatt commented Apr 24, 2025

TheBlueMatt commented Apr 26, 2025

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

codecov bot commented May 5, 2025

Codecov Report

Store `Feature` flags in line rather than on the heap by default (without increasing their size) #3730

Store `Feature` flags in line rather than on the heap by default (without increasing their size) #3730

ldk-reviews-bot commented Apr 10, 2025 •

edited

Loading