Issue Details (XML | Word | Printable)

Key: SAK-14165
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Beth Kirschner
Reporter: Noah Botimer
Votes: 0
Watchers: 3
Operations

If you were logged in you would be able to see more operations.
Sakai

Portfolio assembly is slow with many completed Forms

Created: 07-Aug-2008 13:24   Updated: 15-Dec-2009 13:19
Component/s: Content service (Pre-K1/2.6), OSP: Portfolios
Affects Version/s: 2.5.2, 2.5.3, 2.5.4, 2.5.5, 2.6.0
Fix Version/s: 2.6.1, 2.7.0

Time Tracking:
Not Specified

File Attachments: 1. Text File patch-to-show-only-my-worksite.patch (3 kB)
2. Text File SAK-14165-content-revised.patch (6 kB)
3. Text File SAK-14165-content.patch (5 kB)
4. Text File SAK-14165-metaobj.patch (7 kB)

Issue Links:
Depend

2.6.x Status: Closed
2.5.x Status: None
2.4.x Status: None


 Description  « Hide
The getResourcesOfType method in ContentHostingService (from SAK-11535) helps performance when assembling a portfolio, but still retrieves and checks every completed form in the system for type and permission. This becomes quite slow at scale due to multiple database roundtrips for every form, regardless of user. At Michigan, we are experiencing page load times approaching one minute.

This is a tricky issue because of service boundaries. The StructuredArtifactFinders search for a given type of form, which is a metaobj notion. ContentHostingService and SecurityService only work together to return a permission-checked list of ContentResources of the generic metaobj type.

There are two core issues here:

 1. Every form in the system is checked for permissions before being filtered by type, generally checking many irrelevant items.
 2. Every form is checked for permissions individually by looping rather than by set, issuing many queries.

Resolving the first issue would require ContentHosting to be able to discern between form types, ideally by query, but at least by filtering. This may be resolvable by passing a filtering predicate.

Resolving the second issue would be more involved and needs discussion. At issue is whether checking only collections that are currently available to the user is sufficient, as well the lack of an efficient means to check unlock for a set of references across collections.


The very simple patch attached eliminates one round trip per item by caching the already-retrieved entities before calling unlockCheck on them. This changes the profiled downstream time distribution between SecurityService.unlock() and BaseContentService.availabilityCheck() from roughly 53/47 to 98/0.



 All   Comments   Work Log   Change History   Subversion Commits   git Commits      Sort Order: Ascending order - Click to sort in descending order
Charles Hedrick added a comment - 15-Aug-2008 10:40
Are you sure about this? StructuredArtifactFInder ends up calling contenthostingservice.findResources(null, null, null). That calls getFlatResources on each site that the user has access to. When called with nulls, that just access checks the files. getFlatResources has already done that, so we have 2 access checks. (That can be fixed by calling filterArtifacts with the large argument false.) But as far as I can see it does not check every form of that type on the system, only in sites that the user has access to.


Charles Hedrick added a comment - 15-Aug-2008 10:50
getResourcesOfType doesn't seem to be called. Instead findByOwnerAndType is used. By using Owner, the code is able to restrict access to sites to which the owner has access. I believe this is a result of patches which you may not have.

Portfolio performance issues when selecting items for templates that have many forms as item defs
http://jira.sakaiproject.org/jira/browse/SAK-13791

Slow performance in wizards and viewing portfolios that use matrices
http://jira.sakaiproject.org/jira/browse/SAK-13804

Charles Hedrick added a comment - 17-Aug-2008 09:30
I now believe that the right thing to do is to show only forms and files in the user's worksite. This isn't just an efficiency issue. Rather, if you let users choose things from other sites, their portfolio will break if the other sites are removed or changed. We expect portfolios to have a long life: we will encourage freshmen to start them, and may support them for alumni in some disciplines. Course and other sites tend to have much shorter lifetimes. Thus if a user wants to include a form or file from another site, we'd like them to copy it to their worksite and include it from there. I have removed the code from 14165, going back to the version in SAK-13791, but modified findresources to search only the user's worksite. Please see SAK-13791 for the patch.


Charles Hedrick added a comment - 17-Aug-2008 09:34
I see that SAK-13791 has been closed, with pointers here. So I'm going to attach my patch here. But be aware that to use it you must back out the changes given in this site, returning to the state in SAK-13791


Sean Keesler added a comment - 25-Aug-2008 13:53 - edited
During today's call, we discussed Rutgers solution to SAK-14165 (speeding up the load of the page that builds the "pick lists" of items that could be used in a template based portfolio.

The patch developed by Rutgers restricts this search for potential content to MyWorkspace if a sakai.property is set.

Its reasonable to assume that most content would be created in the process of completed a matrix or wizard and would end up in the "Portfolio Interaction" folder in the student's MyWorkspace. However, the UI still allows for the creation of "form items" right through Resources (assuming that the form is not hidden).

I recommended that an additional sakai.property that turned off the ability to author form items in sites other than MyWorkspace would be a reasonable compliment to this feature (preventing students form authoring content that they would never be able to use).

Another approach would be to use the Rutgers approach as an initial page load to populate the pick lists with content from MyWorkspace, with an option to search for "all" content (as it does right now) for those that need it.

Noah Botimer added a comment - 07-Sep-2008 21:53
The attached patches resolve the majority of the performance issue. They do not change the behavior in that only items owned by the current user within accessible sites are listed in the portfolio assembly screen. Both the content and metaobj patches are required, as there is a signature/implementation change in ContentHostingService.

These patches are against a lightly modified 2.5.2. They should apply quite cleanly to 2.5.x trees.

Noah Botimer added a comment - 07-Sep-2008 22:00
Without significant refactoring of how resources are stored, located, and checked for permissions, this patch is approximately as efficient as possible. It requires a small change to the API of ContentHostingService and came too late, so it will not be included for Kernel 1 / 2.6. It is, however, easy to apply the changes to a custom post-K1 build.

Beth Kirschner added a comment - 08-Sep-2008 06:15
Re-opening until the attached patch is applied to subversion

Noah Botimer added a comment - 14-Apr-2009 07:54
I'm attaching an upgraded patch. It avoids multiple round-trips in getResourcesOfType, which previously happened for each "item definition" in presentation templates. This should yield significant improvements when editing the contents of a templated portfolio.

Beth Kirschner added a comment - 10-Jun-2009 07:41
I've checked in changes that uses the intent of both patches, but also leverages the new ContentHostingService.getContextResourcesOfType() method which should be a lot faster than what currently exists. This new CHS method will retrieve forms only in sites the user is a member -- and this logic is executed within the SQL Select statement (not in the java code). It is likely that the only forms returned will be those in the user's My Workspace, given current usage patterns. This smaller list is then filtered to include only those forms of the requested type and owner.

QA Instructions: Modified code can be exercised in two places:

1) Portfolio -> Add/Edit Content (displaying this page, as long as there are forms to select from, will exercise the new CHS method)

2) Forms -> Edit -> Select Schema File -> Select -> Continue -> Save Changes (this exercises related re-factored code)

Beth Kirschner added a comment - 27-Jul-2009 09:08
Michigan will be testing this in production in the Fall -- once verified & closed, this should be merged to 2.6.x

Charles Hedrick added a comment - 27-Jul-2009 11:44
That looks right. I'm concerned about any search that is proportional to either the number of users or of sites. We need something that is roughly constant in the size of the system.

Charles Hedrick added a comment - 02-Aug-2009 17:56
FYI: Rutgers is installing the June 10, 2009 patch. We'll try them for the next month and decide whether to keep them in the fall.

Noah Botimer added a comment - 05-Oct-2009 10:58
Michigan has been running this in production 2.6.x for a month with no problems. Performance is significantly improved.

Jean-François Lévêque added a comment - 07-Oct-2009 02:56
Looks like something is missing in the r63496 commit for a merge into 2.6.x:
[INFO] Compilation failure
/home/leveque/sakai/source/sakai_2-6-x_full_build/metaobj/metaobj-impl/api-impl/src/java/org/sakaiproject/metaobj/shared/mgt/impl/WrappedStructuredArtifactFinder.java:[96,33] cannot find symbol
symbol : method getContextResourcesOfType(java.lang.String,java.util.Set)
location: interface org.sakaiproject.content.api.ContentHostingService

/home/leveque/sakai/source/sakai_2-6-x_full_build/metaobj/metaobj-impl/api-impl/src/java/org/sakaiproject/metaobj/shared/mgt/impl/WrappedStructuredArtifactFinder.java:[96,33] cannot find symbol
symbol : method getContextResourcesOfType(java.lang.String,java.util.Set)
location: interface org.sakaiproject.content.api.ContentHostingService

Beth Kirschner added a comment - 07-Oct-2009 06:39
This issue cannot be merged to 2.6.x without a new kernel release that includes KNL-210

Matthew Jones added a comment - 07-Oct-2009 08:40
I currently marked the merge as a "Won't fix" as it depends on the merge of the kernel issue, and the fix version for KNL-210 is marked as 1.1.0.

Anthony Whyte added a comment - 12-Oct-2009 09:14
Merged to 2.6.1, r67668.

Beth Kirschner added a comment - 14-Oct-2009 11:34
Merged to 2.6.x