Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions cedar-policy-core/src/ast/entity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use serde_with::{serde_as, TryFromInto};
use smol_str::SmolStr;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::str::FromStr;
use std::sync::Arc;
use thiserror::Error;

/// The entity type that Actions must have
Expand Down Expand Up @@ -657,6 +658,25 @@ impl TCNode<EntityUID> for Entity {
}
}

impl TCNode<EntityUID> for Arc<Entity> {
fn get_key(&self) -> EntityUID {
self.uid().clone()
}

fn add_edge_to(&mut self, k: EntityUID) {
// Use Arc::make_mut to get a mutable reference to the inner value
Arc::make_mut(self).add_ancestor(k)
}

fn out_edges(&self) -> Box<dyn Iterator<Item = &EntityUID> + '_> {
Box::new(self.ancestors())
}

fn has_edge_to(&self, e: &EntityUID) -> bool {
self.is_descendant_of(e)
}
}

impl std::fmt::Display for Entity {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
Expand Down Expand Up @@ -753,6 +773,13 @@ impl From<&Entity> for proto::Entity {
}
}

#[cfg(feature = "protobufs")]
impl From<&Arc<Entity>> for proto::Entity {
fn from(v: &Arc<Entity>) -> Self {
Self::from(v.as_ref())
}
}

/// `PartialValue`, but serialized as a `RestrictedExpr`.
///
/// (Extension values can't be directly serialized, but can be serialized as
Expand Down
21 changes: 13 additions & 8 deletions cedar-policy-core/src/entities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ pub struct Entities {
/// Important internal invariant: for any `Entities` object that exists, the
/// the `ancestor` relation is transitively closed.
#[serde_as(as = "Vec<(_, _)>")]
entities: HashMap<EntityUID, Entity>,
entities: HashMap<EntityUID, Arc<Entity>>,

/// The mode flag determines whether this store functions as a partial store or
/// as a fully concrete store.
Expand Down Expand Up @@ -109,7 +109,7 @@ impl Entities {

/// Iterate over the `Entity`s in the `Entities`
pub fn iter(&self) -> impl Iterator<Item = &Entity> {
self.entities.values()
self.entities.values().map(|e| e.as_ref())
}

/// Adds the [`crate::ast::Entity`]s in the iterator to this [`Entities`].
Expand Down Expand Up @@ -140,7 +140,7 @@ impl Entities {
return Err(EntitiesError::duplicate(entity.uid().clone()))
}
hash_map::Entry::Vacant(vacant_entry) => {
vacant_entry.insert(entity);
vacant_entry.insert(Arc::new(entity));
}
}
}
Expand Down Expand Up @@ -213,7 +213,7 @@ impl Entities {
schema
.action_entities()
.into_iter()
.map(|e| (e.uid().clone(), Arc::unwrap_or_clone(e))),
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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.

.map(|e: Arc<Entity>| (e.uid().clone(), e)),
);
}
Ok(Self {
Expand Down Expand Up @@ -252,6 +252,7 @@ impl Entities {
fn to_ejsons(&self) -> Result<Vec<EntityJson>> {
self.entities
.values()
.map(Arc::as_ref)
.map(EntityJson::from_entity)
.collect::<std::result::Result<_, JsonSerializationError>>()
.map_err(Into::into)
Expand Down Expand Up @@ -322,13 +323,13 @@ impl Entities {
}

/// Create a map from EntityUids to Entities, erroring if there are any duplicates
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>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

let mut map = HashMap::new();
for e in es {
match map.entry(e.uid().clone()) {
hash_map::Entry::Occupied(_) => return Err(EntitiesError::duplicate(e.uid().clone())),
hash_map::Entry::Vacant(v) => {
v.insert(e);
v.insert(Arc::new(e));
}
};
}
Expand All @@ -338,10 +339,14 @@ fn create_entity_map(es: impl Iterator<Item = Entity>) -> Result<HashMap<EntityU
impl IntoIterator for Entities {
type Item = Entity;

type IntoIter = hash_map::IntoValues<EntityUID, Entity>;
type IntoIter = std::vec::IntoIter<Entity>;

fn into_iter(self) -> Self::IntoIter {
self.entities.into_values()
self.entities
.into_values()
.map(Arc::unwrap_or_clone)
.collect::<Vec<Entity>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

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>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely, I'll check that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

.into_iter()
}
}

Expand Down