SEC-593: org.acegisecurity.afterinvocation.AclEntryAfterInvocationCollectionFilteringProvider is slow #854

Closed
spring-issuemaster opened this Issue Nov 6, 2007 · 10 comments

1 participant

@spring-issuemaster

Simon van der Sluis (Migrated from SEC-593) said:

AclEntryAfterInvocationCollectionFilteringProvider is very slow for 2 reasons:

1.) The hasPermission(Authentication, Object) method in AbstractAclProvider is called once for every object in the collection being filtered, which in turn calls aclService.readAclById(..). The net result is that if the Acl cache is empty (or doesn’t yet contain Acls for the domain objects being checked) an Acl query to the database will occur for each item in the Collection. This is inefficient and gets very slow when filtering large collections. Since AclService supports looking up more than one Acl at a time, it makes more sense to collect a batch of ObjectIdentities and perform a lookup on the batch.

2.) CollectionFilterer which is used by AclEntryAfterInvocationCollectionFilteringProvider to remove objects from the Collection being filtered exhibits O(n^2) performance when the collection being filtered has only sequential access (i.e. LinkedList or ArrayList). This is because a Set of objects to be removed from the collection is collected during iteration, and then each is removed when getFilteredObject() is called.

I’ve written a AclEntryAfterInvocationCollectionBatchingFilteringProvider and a FastCollectionFilterer plus some unit tests to solve these problems. In our application when the AclCache is empty filtering ~3500 objects went from ~9sec down to ~2sec.

@spring-issuemaster

Simon van der Sluis said:

Implementation of AclEntryAfterInvocationCollectionBatchingFilteringProvider

@spring-issuemaster

Simon van der Sluis said:

Implementation of FastCollectionFilterer

@spring-issuemaster

Simon van der Sluis said:

Tests for FastCollectionFilterer

@spring-issuemaster

Ben Alex said:

Thanks Simon for the contribution.

My testing of the contributed classes indicate they have different behavior from the existing AclEntryAfterInvocationCollectionFilteringProvider. If you have a chance, please execute your contribution against the tests for the Contacts and DMS samples. You will find it filters a different number of objects. I will take a look at this at a future date if you don’t have an opportunity to do so beforehand.

@spring-issuemaster

Simon van der Sluis said:

That’s odd. I remember a bug where I had some inverted logic, possibly I’ve attached the wrong version of the FastCollectionFilter.

I had a look at the one I attached, it looks a little different from the one we’re using, so I’ve attached the one we’re using -

FastCopyCollectionFilterer.java

Sorry I don’t have any spare time right now to test it against the sample app. Hope this version is better.
I’ve probably got a unit test for it somewhere, let me know if it’s wanted.

Apologies for posting a dud fix.

@spring-issuemaster

Simon van der Sluis said:

Found the unit test

@spring-issuemaster

Ryan Gardner said:

Can I integrate this into a 2.0.4 application or does it have dependencies upon code in the 3.0 version?

@spring-issuemaster

Simon van der Sluis said:

It was written for acegi 1.0.3

I'm guessing with a few minor changes (class names and imports) it would probably work in 2.0.4.

Since it's all based on dependency injection, it's pretty easy to keep the new version in your own code base, and inject it via Spring config.

Be sure to use the correct one FastCopyCollectionFilterer.java, as the first one I attached was buggy.

@spring-issuemaster

Luke Taylor said:

Things have changed a bit for the 3.0 release - it's expected that the most common filtering mechanism wil be using EL, which may or may not include the use of ACLs. This means that the code which is doing the filtering (and deciding what to retain) is unaware of the ACL system when it evaluates a "hasPermission()" expression for a particular object in the collection. We could still probably implement this optimization by adding an extra interface on the AclPermissionEvaluator which would batch load the Acls, e.g.

interface PermissionCacheOptimizer {
cachePermissionsFor(Authentication a, Collection objects);
}

and adding a check in the DefaultMethodexpressionHandler whether its configured PermissionEvaluator also implements this interface.

@spring-issuemaster

Luke Taylor said:

I've implemented the PermissionCacheOptimizer (plus as Acl implementation which batch loads Acls). This is now invoked with the list of objects to be filtered, prior to doing the actual filtering. It should be injected into DefaultMethodSecurityExpressionHandler.

The actual filtering has changed quite a bit, so if additional optimizations are required, please open new issue(s) against the current codebase.

@spring-issuemaster spring-issuemaster added this to the 3.1.0.M1 milestone Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment