Skip to content

perf(jans-cedarling): reduce clones through Arc sharing#14173

Merged
olehbozhok merged 3 commits into
mainfrom
jans-cedarling-14030
Jun 1, 2026
Merged

perf(jans-cedarling): reduce clones through Arc sharing#14173
olehbozhok merged 3 commits into
mainfrom
jans-cedarling-14030

Conversation

@dagregi
Copy link
Copy Markdown
Contributor

@dagregi dagregi commented May 28, 2026

Prepare


Description

Target issue

closes #14030

Implementation Details

I avoided adding ryu::Buffer, SmallVec, and phf since benchmarks showed no measurable difference so pulling in new dependencies wasn't warranted.


Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

Summary by CodeRabbit

  • Refactor
    • Optimized internal memory management and data handling for improved performance and efficiency.

Review Change Stack

Signed-off-by: dagregi <dagmawi.m@proton.me>
@dagregi dagregi self-assigned this May 28, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 28, 2026

📝 Walkthrough

Walkthrough

This PR optimizes the Cedar authorization and entity-building pipeline by restructuring data ownership patterns: default entities move to Arc-wrapped storage, pushed-data transfers ownership rather than being borrowed, and entity builders use stack-allocated arrays and slices for fixed-arity inputs.

Changes

Allocation Optimizations

Layer / File(s) Summary
Default Entities Arc Wrapping
src/common/default_entities.rs, src/common/default_entities_limits.rs
DefaultEntities.inner is wrapped in Arc<HashMap<EntityUid, Entity>> instead of owned directly. Constructors use Arc::new(...) to wrap the map on creation. Validation and test code updated to iterate via entities.inner.iter() and construct Arc-wrapped instances.
Pushed Data Ownership Transfer and Arc-Aware Entity Extraction
src/authz/build_ctx.rs, src/authz/mod.rs
build_context and build_multi_issuer_context now take pushed_data as an owned HashMap<String, Value> (not borrowed), eliminating intermediate clones. Context.data is built directly via serde_json::Map::from_iter(pushed_data). Authorization flows (authorize_multi_issuer, authorize_unsigned) pass pushed-data by value. Default entity extraction uses Arc::try_unwrap to take ownership when uniquely held, otherwise clones.
Entity Building Stack Allocations
src/entity_builder/build_principal_entity.rs, src/entity_builder/build_principal_entity/unsigned.rs, src/entity_builder/build_multi_issuer_entity.rs
Principal entity building accepts attrs_srcs as a borrowed slice &[AttrSrc] instead of owned Vec. Unsigned flow builds id_srcs and attrs_srcs as fixed-size arrays and passes references. Multi-issuer entity building introduces RESERVED_CLAIMS constant for filtering, replaces entity_id_srcs vec! with a single-element array, and refactors all_claims construction to preallocate capacity and extend from the claims iterator.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes


Possibly related PRs

  • JanssenProject/jans#13539: Both PRs modify build_multi_issuer_entity.rs reserved-claims handling via filter_reserved_claims for multi-issuer entity attribute construction.
  • JanssenProject/jans#14084: Both PRs update multi-issuer entity construction in build_multi_issuer_entity.rs, particularly build_single_token_entity logic for entity id source resolution from trusted issuer metadata.

Suggested labels

kind-enhancement, comp-jans-cedarling


Suggested reviewers

  • olehbozhok
  • tareknaser
  • haileyesus2433
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'perf(jans-cedarling): reduce clones through Arc sharing' accurately and concisely summarizes the main change: performance optimization by using Arc to share data and reduce cloning operations.
Description check ✅ Passed The description follows the template structure with Prepare section, Target issue (#14030), Implementation Details explaining why new dependencies were avoided, and Test & Documentation checklist. Author confirms no doc impact and marks static code analysis as completed.
Linked Issues check ✅ Passed The PR addresses issue #14030's stated performance optimization goals: Arc wrapping of DefaultEntities inner HashMap, passing pushed_data by value instead of reference, RESERVED_CLAIMS constant for efficient claim filtering, and SmallVec-like stack allocation patterns for fixed-arity inputs.
Out of Scope Changes check ✅ Passed All changes directly support the linked issue #14030 performance objectives: Arc usage for shared ownership, value vs reference parameter passing, reserved claims optimization, and stack allocation for fixed-arity inputs. No unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jans-cedarling-14030

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 and usage tips.

@mo-auto
Copy link
Copy Markdown
Member

mo-auto commented May 28, 2026

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@mo-auto mo-auto added comp-jans-cedarling Touching folder /jans-cedarling kind-enhancement Issue or PR is an enhancement to an existing functionality labels May 28, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
jans-cedarling/cedarling/src/common/default_entities_limits.rs (1)

1-1: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add the required Apache 2.0 file header.

This Rust file is missing the required Apache 2.0 license header with copyright notice.

Suggested fix
+// This software is available under the Apache-2.0 license.
+// See https://www.apache.org/licenses/LICENSE-2.0.txt for full text.
+//
+// Copyright (c) 2024, Gluu, Inc.
+
 use cedar_policy::entities_errors::EntitiesError;
As per coding guidelines: "Each Rust file must contain the Apache 2.0 license header with copyright notice".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@jans-cedarling/cedarling/src/common/default_entities_limits.rs` at line 1,
Add the required Apache 2.0 license header and copyright notice to the top of
the file that currently contains the line "use
cedar_policy::entities_errors::EntitiesError;"; insert the standard Apache 2.0
file header (including copyright holder and license text/URL) as the very first
lines of cedarling/src/common/default_entities_limits.rs so the header precedes
the existing use statement and any code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@jans-cedarling/cedarling/src/common/default_entities_limits.rs`:
- Line 1: Add the required Apache 2.0 license header and copyright notice to the
top of the file that currently contains the line "use
cedar_policy::entities_errors::EntitiesError;"; insert the standard Apache 2.0
file header (including copyright holder and license text/URL) as the very first
lines of cedarling/src/common/default_entities_limits.rs so the header precedes
the existing use statement and any code.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 56ed7df6-7616-46a3-bc98-934c79d380da

📥 Commits

Reviewing files that changed from the base of the PR and between 97a9e04 and 9883f00.

📒 Files selected for processing (7)
  • jans-cedarling/cedarling/src/authz/build_ctx.rs
  • jans-cedarling/cedarling/src/authz/mod.rs
  • jans-cedarling/cedarling/src/common/default_entities.rs
  • jans-cedarling/cedarling/src/common/default_entities_limits.rs
  • jans-cedarling/cedarling/src/entity_builder/build_multi_issuer_entity.rs
  • jans-cedarling/cedarling/src/entity_builder/build_principal_entity.rs
  • jans-cedarling/cedarling/src/entity_builder/build_principal_entity/unsigned.rs

Copy link
Copy Markdown
Contributor

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Thank you

Comment thread jans-cedarling/cedarling/src/authz/mod.rs
@olehbozhok olehbozhok merged commit ffd314f into main Jun 1, 2026
15 of 16 checks passed
@olehbozhok olehbozhok deleted the jans-cedarling-14030 branch June 1, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp-jans-cedarling Touching folder /jans-cedarling kind-enhancement Issue or PR is an enhancement to an existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(jans-cedarling/authz): Cedar-native context build, Arc claim snapshots, ryu decimals in value_to_expr

5 participants