Skip to content

[FEAT] De-box Source::next — the boxed future buys erasure nobody uses #254

Description

@lxsaah

Is your feature request related to a problem? Please describe.

Source::next (aimdb-core/src/session/mod.rs:530) returns a BoxFut, so every implementor heap-allocates once per inbound message:

fn next(&mut self) -> BoxFut<'_, Option<(String, Payload)>>;

It is called from the pump_source read loop at aimdb-core/src/session/pump.rs:152, i.e. once per received telegram/message, on every inbound data path in the tree.

Unlike the outbound side, this allocation is not paying for anything. pump_sink holds Arc<dyn Connector> (pump.rs:40), so Connector::publish genuinely has to return a boxed future — that one is the price of the pluggable connector surface and should stay. But pump_source takes mut src: impl Source + 'static (pump.rs:141) — generic and monomorphized, no trait object anywhere. The only two dyn Source mentions in the workspace are the object-safety assertion itself (mod.rs:546) and a test (mod.rs:651). No production code erases a Source, so the box is satisfying a dyn-compatibility constraint that no caller exercises.

This is a code-cleanliness issue, not a performance one. At current connector rates (KNX TP1 is 9600 baud; IP tunnelling runs a single gateway channel) one small allocation per message is nowhere near a bottleneck, and on the inbound path it is outnumbered anyway — aimdb-knx-connector/src/client.rs:80 already does an addr.to_string() plus a Vec<u8>Arc<[u8]> copy per telegram, both larger than the box. Filing so the reasoning is recorded rather than rediscovered; no urgency.

Describe the solution you'd like

Switch the trait to AFIT, matching the shape already used elsewhere in the tree:

fn next(&mut self) -> impl Future<Output = Option<(String, Payload)>> + Send + '_;

Two in-repo precedents show this works and needs no MSRV movement:

  • CommandSource (aimdb-knx-connector/src/client.rs:43) is already written this way.
  • EmbassySourceRaw (aimdb-embassy-adapter/src/connectors.rs:99) is already in the target shape, and its EmbassySource bridge boxes purely to satisfy core's BoxFut (connectors.rs:113). Since SendFutureWrapper<F> is unconditionally Send (aimdb-embassy-adapter/src/send_wrapper.rs:25), that line collapses to SendFutureWrapper(self.0.next()) with the box gone and nothing else moving — the allocation-sensitive MCU target is the one where the change is most mechanical.

Blast radius is three production impls plus core's own test scaffolding:

Site What it is
aimdb-core/src/session/mod.rs:530 the trait declaration
aimdb-core/src/session/mod.rs:546 _source: &dyn Source in _assert_object_safe — must be dropped
aimdb-core/src/session/mod.rs:637,651 MockSource and its Box<dyn Source> test
aimdb-embassy-adapter/src/connectors.rs:112 EmbassySource<S>
aimdb-knx-connector/src/connector.rs:74 KnxSource
aimdb-mqtt-connector/src/tokio_client.rs:318 MqttEventLoopSource

pump_source itself needs no change — it is already generic over impl Source.

Note that a grep for BoxFut looks far more alarming than this change is: nearly all hits are Connection, Dialer, Listener and Dispatch, which are genuinely stored as Box<dyn …> throughout the session engine and are untouched here. Source is the one data-plane trait that declares a boxed future without anyone erasing it.

Describe alternatives you've considered

Leave it as is — the current decision, and defensible. Source mirroring Connector keeps the two data-plane halves symmetric, and keeping the trait dyn-compatible leaves the door open to a future dyn Source holder (a multiplexer over heterogeneous sources, say). If that door is one we want open, the box is the cost of holding it — worth making that an explicit decision rather than an inherited one, which is the main reason for this issue.

Reduce the surrounding allocations first — on the inbound KNX path, interning the group-address topic (it comes from a fixed set of GroupAddress values) and parsing straight into an Arc<[u8]> would each save more per telegram than de-boxing. Independent of this issue and a better payoff if allocation pressure ever actually matters.

Additional context

The blocking consideration is API breakage, not the in-tree edits. Source is publicly re-exported from aimdb-core/src/lib.rs:143, so making it non-dyn-compatible breaks any out-of-tree connector holding a Box<dyn Source>, with no deprecation path for that case. An out-of-tree impl that merely returns a future keeps compiling. In-tree this is an afternoon; as public API it wants a version boundary, so the natural time to do it is alongside the next breaking release of aimdb-core.

Worth revisiting sooner if a genuinely high-rate connector lands on pump_source, at which point the inbound allocation profile becomes worth real effort.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions