Skip to content

perf(jid): compare structured JID attributes without rendering them - #1176

Merged
jlucaso1 merged 1 commit into
mainfrom
perf/valueref-jid-no-stringify
Jul 29, 2026
Merged

perf(jid): compare structured JID attributes without rendering them#1176
jlucaso1 merged 1 commit into
mainfrom
perf/valueref-jid-no-stringify

Conversation

@jlucaso1

Copy link
Copy Markdown
Collaborator

A decoded JID attribute is kept structured as ValueRef::Jid. Comparing it to a literal via v.as_str() == SERVER_JID throws that away: as_str() renders the JID into an owned String purely to be compared and dropped. ValueRef already implements PartialEq<str>, which walks the display form against the needle without building it.

Applied to the server-origin checks in pair.rs, passkey/flow.rs (×2) and device-notification handling, plus NodeFilter's attribute match.

Equivalence

Two of these gate whether a stanza is honored at all ("only honor a server request"), so this is not a place to assume the two forms agree — I checked. comparing_a_jid_value_matches_comparing_its_rendered_form asserts v == needle matches v.as_str() == needle across user, device, agent, group, lid and broadcast shapes, for both the Jid and String variants, and specifically for the server-only JID (s.whatsapp.net, empty user) — which is the shape those checks actually compare against, and the one the fast parser declines to handle.

Deliberately not included

The same sweep also proposed node.get_attr("from").and_then(|v| v.as_str().parse().ok())v.to_jid() for ServerAck. I left that out: measured against the old form, it is not equivalent. Display omits the agent for servers where renders_agent() is false, so stringify-then-reparse silently drops it while to_jid() preserves it:

12345.6@hosted.lid
  old (stringify+parse): agent = 0
  new (to_jid):          agent = 6

to_jid() is arguably the more faithful reading — the agent came off the wire — but it changes an observable field of a published event, and Event payloads are a frozen API. That belongs in its own change with its own justification, not smuggled into a perf PR.

Found by a two-model performance sweep (Claude Opus 5 and Codex gpt-5.6-sol). Codex measured the comparison shortcut in isolation; note these are low-frequency control-flow paths, so the aggregate gain is modest — the durable value is not undoing the structured representation the decoder went to the trouble of keeping.

`ValueRef::as_str()` renders a `Jid` variant into an owned `String` just
so the caller can compare it to a literal. `ValueRef` already implements
`PartialEq<str>`, which walks the JID's display form against the needle
without building it.

Applied to the server-origin checks in pair, passkey and device
notification handling, and to `NodeFilter`'s attribute match.

Equivalence is what matters here, since two of these gate whether a
stanza is honored at all: `comparing_a_jid_value_matches_comparing_its_rendered_form`
asserts the two forms agree over user/device/agent/group/lid shapes and
the server-only JID that those checks actually compare against.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 44218f64-94fa-49cd-b125-920f2e6b3371

📥 Commits

Reviewing files that changed from the base of the PR and between 83e7b89 and 887694a.

📒 Files selected for processing (5)
  • src/client.rs
  • src/handlers/notification/device.rs
  • src/pair.rs
  • src/passkey/flow.rs
  • wacore/binary/src/node.rs

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation of server-targeted notifications and incoming requests.
    • Ensured attribute matching works consistently for JIDs, strings, and invalid values.
  • Tests

    • Added coverage for comparing node values directly with string values, including server-only and invalid JIDs.

Walkthrough

The PR replaces redundant string conversions with direct ValueRef comparisons in node filtering and server-JID validation, and adds tests covering JID and string equality behavior.

Changes

ValueRef matching updates

Layer / File(s) Summary
Direct attribute and server-JID comparisons
src/client.rs, src/handlers/notification/device.rs, src/pair.rs, src/passkey/flow.rs
Node filtering and notification, IQ, and passkey validation compare attribute values directly against expected strings or SERVER_JID.
ValueRef equality tests
wacore/binary/src/node.rs
Tests cover ValueRef::Jid and ValueRef::String comparisons, including server-only JIDs and invalid inputs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: performance

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main optimization: comparing structured JID attributes without rendering them.
Description check ✅ Passed The description matches the changeset and explains the JID comparison optimization and related test coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/valueref-jid-no-stringify

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jlucaso1
jlucaso1 merged commit cbbdbda into main Jul 29, 2026
21 of 23 checks passed
@jlucaso1
jlucaso1 deleted the perf/valueref-jid-no-stringify branch July 29, 2026 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant