Speed Improvement: Make incoming entities Arc<Entities> like schema entities #1296

tomlikestorock · 2024-10-29T00:46:16Z

Description of changes

This provides a 3x speedup based on local benchmarking with a large schema that has big action entity hierarchy relationships. Today Entities.entities is a HashMap of <EntityUID, Entity>. When from_entities brings in the entities and actions from a given schema, it currently relies on Arc::unwrap_or_clone, which frequently ends up being clone. For a large set of very interrelated Actions, this repeated clone becomes expensive.

Changing Entities.entities to be a HashMap of <EntityUID, Arc<Entity>> allows us to keep the clone cheap, and Arc'ing the incoming Entities becomes cheap as well.

Changing the interface of the HashMap means altering what create_entities_map returns, adding an implementation of TCNode for Arc<Entity>, and udpating how IntoIterator works to return values from the Arc's.

Issue #, if available

This contributes to 1285.

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

I confirm that this PR (choose one, and delete the other options):

Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

Does not require updates because my change does not impact the Cedar language specification.

cdisselkoen · 2024-10-29T12:46:06Z

cedar-policy-core/src/entities.rs

-fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityUID, Entity>> {
+fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityUID, Arc<Entity>>> {


should we also have the input es here be an iterator of Arc<Entity>, and force callers to map Arc::new if needed?

The same concern also applies to add_entities, right? I'd be in favor of making our internal APIs more explicit by taking Arc on these functions. Public APIs shouldn't change ofc.

Ah, I wasn't sure how interested you'd be in propagating this Arc expectation (at least internally). I can make change that reflect this for sure.

Updated to this request in 1da976e, let me know what you think.

cdisselkoen · 2024-10-29T12:48:14Z

cedar-policy-core/src/entities.rs

+        self.entities
+            .into_values()
+            .map(Arc::unwrap_or_clone)
+            .collect::<Vec<Entity>>()


Having to .collect() here seems like a potential performance problem as well ... is it possible to avoid it with a different choice of type IntoIter above, so that the implementation here can just be self.entities.into_values().map(Arc::unwrap_or_clone)? or alternately, should we change the type Item to be Arc<Entity>?

Definitely, I'll check that out.

Updated in 11e70bd. I changed the IntoIter type to reflect the .map call as the return type. This gave another speed boost as is.

john-h-kastner-aws

Looks good. Craig's comments are probably worth addressing.

john-h-kastner-aws · 2024-10-29T13:41:33Z

cedar-policy-core/src/entities.rs

-fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityUID, Entity>> {
+fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityUID, Arc<Entity>>> {


The same concern also applies to add_entities, right? I'd be in favor of making our internal APIs more explicit by taking Arc on these functions. Public APIs shouldn't change ofc.

john-h-kastner-aws · 2024-10-29T14:03:41Z

cedar-policy-core/src/entities.rs

                schema
                    .action_entities()
                    .into_iter()
-                    .map(|e| (e.uid().clone(), Arc::unwrap_or_clone(e))),


It took me a second to puzzle out why this actually gives a speed up, so I'll leave my reasoning here. Maybe worth elaborating in a comment.

We remove this unwrap_or_clone, but add a make_mut (which I assume must clone if there's another reference to the data) call inside TC computation, so at first glance it feels like that should be a wash. But, the TC was already computed for the entity map (which we just constructed, so one ref to each Arc), and we separately computed TC for the actions inside the schema constructor (while it had unique ownership). Put together these should mean we never trigger a clone on the make_mut during TC computation.

…rc the incoming entities, update function parameters to receive Arc entities, implement TCNode for Arc<Entity> Signed-off-by: Thomas Hill <[email protected]>

…ty> and proto::Entity Signed-off-by: Thomas Hill <[email protected]>

… receive Arc<Entities> instead, update all tests to take this into account, update call sites of these functions to map to Arc<Entity> as well Signed-off-by: Thomas Hill <[email protected]>

…llecting by updating the IntoIter type, and remove .collect(). Signed-off-by: Thomas Hill <[email protected]>

cdisselkoen · 2024-11-07T13:34:13Z

Thanks for this work!

john-h-kastner-aws

Looks the build error is coming from the protobuf feature

…ped entities into account. Signed-off-by: Thomas Hill <[email protected]>

tomlikestorock · 2024-11-07T19:09:38Z

Protobuf tests updated to use Arc wrapped entities in 857509b.

Signed-off-by: Thomas Hill <[email protected]>

…ntities (#1296) Signed-off-by: Thomas Hill <[email protected]>

…ntities (#1296) Signed-off-by: Thomas Hill <[email protected]> Signed-off-by: Craig Disselkoen <[email protected]>

tpaulus mentioned this pull request Oct 29, 2024

Potential inefficient use of Schema during entity loading #1285

Closed

2 tasks

tpaulus approved these changes Oct 29, 2024

View reviewed changes

cdisselkoen approved these changes Oct 29, 2024

View reviewed changes

john-h-kastner-aws approved these changes Oct 29, 2024

View reviewed changes

tpaulus approved these changes Nov 6, 2024

View reviewed changes

tomlikestorock force-pushed the arc-entities branch from 12fbc39 to 2776826 Compare November 6, 2024 18:31

tomlikestorock added 2 commits November 6, 2024 10:33

Update entities hashmap to hold Arc<Entity>, change add_entities to A…

9a6deeb

…rc the incoming entities, update function parameters to receive Arc entities, implement TCNode for Arc<Entity> Signed-off-by: Thomas Hill <[email protected]>

Implement protobuf feature specific From conversion between &Arc<Enti…

98cc078

…ty> and proto::Entity Signed-off-by: Thomas Hill <[email protected]>

tomlikestorock force-pushed the arc-entities branch from 2776826 to 98cc078 Compare November 6, 2024 18:36

tpaulus approved these changes Nov 6, 2024

View reviewed changes

tomlikestorock added 2 commits November 6, 2024 16:45

Update create_entity_map and core/entities::Entities::add_entities to…

1da976e

… receive Arc<Entities> instead, update all tests to take this into account, update call sites of these functions to map to Arc<Entity> as well Signed-off-by: Thomas Hill <[email protected]>

Update core/entities::IntoIterator for Entities to iterate without co…

11e70bd

…llecting by updating the IntoIter type, and remove .collect(). Signed-off-by: Thomas Hill <[email protected]>

tomlikestorock requested review from john-h-kastner-aws and cdisselkoen November 7, 2024 01:04

cdisselkoen approved these changes Nov 7, 2024

View reviewed changes

john-h-kastner-aws approved these changes Nov 7, 2024

View reviewed changes

Update cedar-policy-core/src/entities protobuf tests to take Arc wrap…

857509b

…ped entities into account. Signed-off-by: Thomas Hill <[email protected]>

tomlikestorock requested review from john-h-kastner-aws and cdisselkoen November 7, 2024 19:14

Formatting changes to be cargo fmt compliant.

cef4be4

Signed-off-by: Thomas Hill <[email protected]>

john-h-kastner-aws approved these changes Nov 7, 2024

View reviewed changes

john-h-kastner-aws merged commit a81e0b9 into cedar-policy:main Nov 7, 2024
19 checks passed

cdisselkoen pushed a commit that referenced this pull request Nov 8, 2024

Speed Improvement: Make incoming entities Arc<Entities> like schema e…

96cec46

…ntities (#1296) Signed-off-by: Thomas Hill <[email protected]>

cdisselkoen mentioned this pull request Nov 8, 2024

Three more cherrypicks to 4.2.x #1309

Merged

cdisselkoen pushed a commit that referenced this pull request Nov 8, 2024

Speed Improvement: Make incoming entities Arc<Entities> like schema e…

51fac17

…ntities (#1296) Signed-off-by: Thomas Hill <[email protected]> Signed-off-by: Craig Disselkoen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Speed Improvement: Make incoming entities Arc<Entities> like schema entities #1296

Speed Improvement: Make incoming entities Arc<Entities> like schema entities #1296

Uh oh!

tomlikestorock commented Oct 29, 2024 •

edited

Loading

Uh oh!

cdisselkoen Oct 29, 2024

Uh oh!

john-h-kastner-aws Oct 29, 2024 •

edited

Loading

Uh oh!

tomlikestorock Oct 29, 2024

Uh oh!

tomlikestorock Nov 7, 2024

Uh oh!

cdisselkoen Oct 29, 2024

Uh oh!

tomlikestorock Oct 29, 2024

Uh oh!

tomlikestorock Nov 7, 2024

Uh oh!

john-h-kastner-aws left a comment

Uh oh!

john-h-kastner-aws Oct 29, 2024 •

edited

Loading

Uh oh!

john-h-kastner-aws Oct 29, 2024 •

edited

Loading

Uh oh!

cdisselkoen commented Nov 7, 2024

Uh oh!

john-h-kastner-aws left a comment

Uh oh!

tomlikestorock commented Nov 7, 2024

Uh oh!

Uh oh!

Uh oh!

		fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityUID, Entity>> {
		fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityUID, Arc<Entity>>> {

Speed Improvement: Make incoming entities Arc<Entities> like schema entities #1296

Speed Improvement: Make incoming entities Arc<Entities> like schema entities #1296

Uh oh!

Conversation

tomlikestorock commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue #, if available

Checklist for requesting a review

Uh oh!

cdisselkoen Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomlikestorock Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

tomlikestorock Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

cdisselkoen Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

tomlikestorock Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

tomlikestorock Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdisselkoen commented Nov 7, 2024

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

tomlikestorock commented Nov 7, 2024

Uh oh!

Uh oh!

Uh oh!

tomlikestorock commented Oct 29, 2024 •

edited

Loading

john-h-kastner-aws Oct 29, 2024 •

edited

Loading

john-h-kastner-aws Oct 29, 2024 •

edited

Loading

john-h-kastner-aws Oct 29, 2024 •

edited

Loading