New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AutorouteService.PublishAlias() causes ALL the AliasRecords to be fetched in the end #4751

Closed
orchardbot opened this Issue Sep 3, 2014 · 13 comments

Comments

Projects
None yet
3 participants
@orchardbot

orchardbot commented Sep 3, 2014

@Piedone created:
https://orchard.codeplex.com/workitem/20922

When AutorouteService.PublishAlias() is called it in turn calls DefaultAliasService.Replace(). This method through Lookup() causes List() to be called what in the end fetches all the records through AliasStorage from the AliasRecord table. To add insult to injury this whole dataset is then filtered in memory in DefaultAliasService.Lookup().

This is exactly what happens when a content item is published, see a corresponding full stack trace at the bottom.

Obviously this causes timeouts in case of large datasets (noticed this with ~40k AliasRecords but this should happen with smaller datasets too even with a 30s timeout value) if anything else tries to access this same table (what inevitably happens).

I have no good solution for this right now. Somehow we should change DefaultAliasService.Lookup() not to fetch everything. For example we could make AliasStorage be able to do exact lookups on he value of RouteValues (which column should then be indexed too). Or we could have an external index table for specific lookup types, as it is with infoset-stored data (think fields and Projector).
Orchard.Alias.dll!Orchard.Alias.Implementation.Storage.AliasStorage.ToDictionary(Orchard.Alias.Records.AliasRecord aliasRecord) Line 152 C#
System.Core.dll!System.Linq.Enumerable.WhereSelectEnumerableIterator<Orchard.Alias.Records.AliasRecord,System.Tuple<string,string,System.Collections.Generic.IDictionary<string,string>,string,int>>.MoveNext() Unknown
mscorlib.dll!System.Collections.Generic.List<System.Tuple<string,string,System.Collections.Generic.IDictionary<string,string>,string,int>>.List(System.Collections.Generic.IEnumerable<System.Tuple<string,string,System.Collections.Generic.IDictionary<string,string>,string,int>> collection) Unknown
System.Core.dll!System.Linq.Enumerable.ToList<System.Tuple<string,string,System.Collections.Generic.IDictionary<string,string>,string,int>>(System.Collections.Generic.IEnumerable<System.Tuple<string,string,System.Collections.Generic.IDictionary<string,string>,string,int>> source) Unknown
Orchard.Alias.dll!Orchard.Alias.Implementation.Storage.AliasStorage.List(System.Linq.Expressions.Expression<System.Func<Orchard.Alias.Records.AliasRecord,bool>> predicate) Line 140 C#
Orchard.Alias.dll!Orchard.Alias.Implementation.Storage.AliasStorage.List() Line 130 C#
Orchard.Alias.dll!Orchard.Alias.Implementation.DefaultAliasService.List() Line 86 C#
Orchard.Alias.dll!Orchard.Alias.Implementation.DefaultAliasService.Lookup(System.Web.Routing.RouteValueDictionary routeValues) Line 82 C#
Orchard.Alias.dll!Orchard.Alias.Implementation.DefaultAliasService.Replace(string aliasPath, System.Web.Routing.RouteValueDictionary routeValues, string aliasSource) Line 71 C#
Orchard.Autoroute.dll!Orchard.Autoroute.Services.AutorouteService.PublishAlias(Orchard.Autoroute.Models.AutoroutePart part) Line 72 C#
Orchard.Autoroute.dll!Orchard.Autoroute.Handlers.AutoroutePartHandler.PublishAlias(Orchard.Autoroute.Models.AutoroutePart part) Line 74 C#
Orchard.Autoroute.dll!Orchard.Autoroute.Handlers.AutoroutePartHandler..ctor.AnonymousMethod__2(Orchard.ContentManagement.Handlers.PublishContentContext ctx, Orchard.Autoroute.Models.AutoroutePart part) Line 39 C#
Orchard.Framework.dll!Orchard.ContentManagement.Handlers.ContentHandler.InlineStorageFilter<Orchard.Autoroute.Models.AutoroutePart>.Published(Orchard.ContentManagement.Handlers.PublishContentContext context, Orchard.Autoroute.Models.AutoroutePart instance) Line 164 C#
Orchard.Framework.dll!Orchard.ContentManagement.Handlers.StorageFilterBase<Orchard.Autoroute.Models.AutoroutePart>.Orchard.ContentManagement.Handlers.IContentStorageFilter.Published(Orchard.ContentManagement.Handlers.PublishContentContext context) Line 87 C#
Orchard.Framework.dll!Orchard.ContentManagement.Handlers.ContentHandler.Orchard.ContentManagement.Handlers.IContentHandler.Published(Orchard.ContentManagement.Handlers.PublishContentContext context) Line 288 C#
Orchard.Framework.dll!Orchard.ContentManagement.DefaultContentManager.Publish.AnonymousMethod__40(Orchard.ContentManagement.Handlers.IContentHandler handler) Line 386 C#
Orchard.Framework.dll!Orchard.InvokeExtensions.Invoke<Orchard.ContentManagement.Handlers.IContentHandler>(System.Collections.Generic.IEnumerable<Orchard.ContentManagement.Handlers.IContentHandler> events, System.Action<Orchard.ContentManagement.Handlers.IContentHandler> dispatch, Orchard.Logging.ILogger logger) Line 17 C#

Orchard.Framework.dll!Orchard.ContentManagement.DefaultContentManager.Publish(Orchard.ContentManagement.ContentItem contentItem) Line 386 C#

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 4, 2014

@CSurieux commented:

What is doing vnext ?

orchardbot commented Sep 4, 2014

@CSurieux commented:

What is doing vnext ?

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 4, 2014

@Piedone commented:

Sorry? This is in the latest source.

orchardbot commented Sep 4, 2014

@Piedone commented:

Sorry? This is in the latest source.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 4, 2014

@CSurieux commented:

Forget my comment, I missed the real point here. :)
(I was looking to route tables implementation in vnext vs orchard's one)

orchardbot commented Sep 4, 2014

@CSurieux commented:

Forget my comment, I missed the real point here. :)
(I was looking to route tables implementation in vnext vs orchard's one)

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 8, 2014

@Piedone commented:

So my best idea is to have a lookup table that would contain a record for each route value corresponding to each alias. E.g. AliasRecord with two route values -> lookup record 1 and 2.

This would allow us to do exact matches, i.e. "I'm looking for all alias records corresponding to this route value".

Simpler would be to do straight lookups on AliasRecord.RouteValues. However this column contains serialized values. So even if we play smart try to look up serialized values as string we can't really do better than end up with a LIKE query in the end, what is less than ideal.

orchardbot commented Sep 8, 2014

@Piedone commented:

So my best idea is to have a lookup table that would contain a record for each route value corresponding to each alias. E.g. AliasRecord with two route values -> lookup record 1 and 2.

This would allow us to do exact matches, i.e. "I'm looking for all alias records corresponding to this route value".

Simpler would be to do straight lookups on AliasRecord.RouteValues. However this column contains serialized values. So even if we play smart try to look up serialized values as string we can't really do better than end up with a LIKE query in the end, what is less than ideal.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 8, 2014

@sebastienros commented:

Look at the history of the module to see if a global lookup has not been removed on purpose (I think so). All the aliases are also in the RouteCollection, that could help. Another approach could be to have a denormalized table that can be queried.

orchardbot commented Sep 8, 2014

@sebastienros commented:

Look at the history of the module to see if a global lookup has not been removed on purpose (I think so). All the aliases are also in the RouteCollection, that could help. Another approach could be to have a denormalized table that can be queried.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 8, 2014

@Piedone commented:

Thanks.

I didn't see global lookup being removed, not even here: https://orchardalias.codeplex.com/

I like using the RouteCollection to do this lookup. It's already in memory and will be fast. The only issue I can think of is that in a multi-server environment it's only as up-to-date as the AliasHolderUpdater can refresh it (once a minute).

This is probably an acceptable compromise and also we could make AliasHolderUpdater better: e.g. we could factor out all the refresh logic to an "AliasUpdater", leaving only a method call in an "AliasUpdaterTask"'s Sweep(). This way if you have a faster solution than using a background task (e.g. by building on Distributed Events: https://orcharddistributedevents.codeplex.com/) then you can implement it, but as a good enough default there will be the background task.

How do you mean the denormalized table?

orchardbot commented Sep 8, 2014

@Piedone commented:

Thanks.

I didn't see global lookup being removed, not even here: https://orchardalias.codeplex.com/

I like using the RouteCollection to do this lookup. It's already in memory and will be fast. The only issue I can think of is that in a multi-server environment it's only as up-to-date as the AliasHolderUpdater can refresh it (once a minute).

This is probably an acceptable compromise and also we could make AliasHolderUpdater better: e.g. we could factor out all the refresh logic to an "AliasUpdater", leaving only a method call in an "AliasUpdaterTask"'s Sweep(). This way if you have a faster solution than using a background task (e.g. by building on Distributed Events: https://orcharddistributedevents.codeplex.com/) then you can implement it, but as a good enough default there will be the background task.

How do you mean the denormalized table?

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 9, 2014

@sebastienros commented:

"denormalized": you mentioned the database schema was not adapted for this kind of query, you could save an extracted view of the data with just what you need for querying purposes.

orchardbot commented Sep 9, 2014

@sebastienros commented:

"denormalized": you mentioned the database schema was not adapted for this kind of query, you could save an extracted view of the data with just what you need for querying purposes.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 9, 2014

@Piedone commented:

OK so this is what I meant by lookup table. I'll start with using RouteCollection then let's see.

orchardbot commented Sep 9, 2014

@Piedone commented:

OK so this is what I meant by lookup table. I'll start with using RouteCollection then let's see.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 10, 2014

@Piedone commented:

Fixed in changeset 8818202

orchardbot commented Sep 10, 2014

@Piedone commented:

Fixed in changeset 8818202

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 10, 2014

@Piedone commented:

I came to the conclusion that given RouteCollection you can only lookup up the alias (or rather, route) that has the highest priority and corresponds to a RouteValueDictionary. This is what we ultimately want, minus the "with the highest priority" part, so this is not usable since it would only return one, but not all aliases (what we need, since the same route values can have multiple aliases).

So I implemented a solution that uses AliasMap. I pushed the fix but I'm not sure about having the if (routeValues.ContainsKey("area")) block in Lookup(): probably this is an unnecessary or wrong constraint?

The implementation is not theoretically optimal (still has and O(n) component but n is the number of modules) but it now uses data already available in memory and is much faster: 6s vs 25ms with my test dataset of ~40k items (and the if() block being there; without it 27ms), on my machine.

Please review. I also did the mentioned refactoring of alias updater so there can be implementation for refreshing aliases more frequently then a background task.

On a side note I'd argue that the logic that AutorouteService.PublishAlias() calling AliasService.Replace() is not right: this means that if a content item changes its permalink any other aliases defined for it will be wiped. I think this defeats the purpose of aliases, i.e. that you can have multiple ones pointing to the same route values. Instead IMO only the alias corresponding to the permalink should be deleted, not all aliases. An implementation like this wouldn't need AliasService.Lookup(RouteValueDictionary routeValues) at all.

orchardbot commented Sep 10, 2014

@Piedone commented:

I came to the conclusion that given RouteCollection you can only lookup up the alias (or rather, route) that has the highest priority and corresponds to a RouteValueDictionary. This is what we ultimately want, minus the "with the highest priority" part, so this is not usable since it would only return one, but not all aliases (what we need, since the same route values can have multiple aliases).

So I implemented a solution that uses AliasMap. I pushed the fix but I'm not sure about having the if (routeValues.ContainsKey("area")) block in Lookup(): probably this is an unnecessary or wrong constraint?

The implementation is not theoretically optimal (still has and O(n) component but n is the number of modules) but it now uses data already available in memory and is much faster: 6s vs 25ms with my test dataset of ~40k items (and the if() block being there; without it 27ms), on my machine.

Please review. I also did the mentioned refactoring of alias updater so there can be implementation for refreshing aliases more frequently then a background task.

On a side note I'd argue that the logic that AutorouteService.PublishAlias() calling AliasService.Replace() is not right: this means that if a content item changes its permalink any other aliases defined for it will be wiped. I think this defeats the purpose of aliases, i.e. that you can have multiple ones pointing to the same route values. Instead IMO only the alias corresponding to the permalink should be deleted, not all aliases. An implementation like this wouldn't need AliasService.Lookup(RouteValueDictionary routeValues) at all.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 10, 2014

@StanleyGoldman commented:

Cooking a site that normally works after changes 8818202 and a3053c3

2014-09-10 15:23:31,670 [11] Orchard.ContentManagement.DefaultContentManager - Default - NullReferenceException thrown from IContentHandler by Orchard.Autoroute.Handlers.AutoroutePartHandler
 http://localhost:8088/
System.NullReferenceException: Object reference not set to an instance of an object.
   at Orchard.Alias.Implementation.DefaultAliasService.Lookup(RouteValueDictionary routeValues)
   at Orchard.Alias.Implementation.DefaultAliasService.Replace(String aliasPath, RouteValueDictionary routeValues, String aliasSource)
   at Orchard.Autoroute.Services.AutorouteService.PublishAlias(AutoroutePart part)
   at Orchard.Autoroute.Handlers.AutoroutePartHandler.PublishAlias(AutoroutePart part)
   at Orchard.Autoroute.Handlers.AutoroutePartHandler.<.ctor>b__2(PublishContentContext ctx, AutoroutePart part)
   at Orchard.ContentManagement.Handlers.ContentHandler.InlineStorageFilter`1.Published(PublishContentContext context, TPart instance) in \src\Orchard\ContentManagement\Handlers\ContentHandler.cs:line 164
   at Orchard.ContentManagement.Handlers.StorageFilterBase`1.Orchard.ContentManagement.Handlers.IContentStorageFilter.Published(PublishContentContext context) in \src\Orchard\ContentManagement\Handlers\StorageFilterBase.cs:line 87
   at Orchard.ContentManagement.Handlers.ContentHandler.Orchard.ContentManagement.Handlers.IContentHandler.Published(PublishContentContext context) in \src\Orchard\ContentManagement\Handlers\ContentHandler.cs:line 288
   at Orchard.ContentManagement.DefaultContentManager.<>c__DisplayClass42.<Publish>b__40(IContentHandler handler) in \src\Orchard\ContentManagement\DefaultContentManager.cs:line 386
   at Orchard.InvokeExtensions.Invoke[TEvents](IEnumerable`1 events, Action`1 dispatch, ILogger logger) in \src\Orchard\InvokeExtensions.cs:line 17

orchardbot commented Sep 10, 2014

@StanleyGoldman commented:

Cooking a site that normally works after changes 8818202 and a3053c3

2014-09-10 15:23:31,670 [11] Orchard.ContentManagement.DefaultContentManager - Default - NullReferenceException thrown from IContentHandler by Orchard.Autoroute.Handlers.AutoroutePartHandler
 http://localhost:8088/
System.NullReferenceException: Object reference not set to an instance of an object.
   at Orchard.Alias.Implementation.DefaultAliasService.Lookup(RouteValueDictionary routeValues)
   at Orchard.Alias.Implementation.DefaultAliasService.Replace(String aliasPath, RouteValueDictionary routeValues, String aliasSource)
   at Orchard.Autoroute.Services.AutorouteService.PublishAlias(AutoroutePart part)
   at Orchard.Autoroute.Handlers.AutoroutePartHandler.PublishAlias(AutoroutePart part)
   at Orchard.Autoroute.Handlers.AutoroutePartHandler.<.ctor>b__2(PublishContentContext ctx, AutoroutePart part)
   at Orchard.ContentManagement.Handlers.ContentHandler.InlineStorageFilter`1.Published(PublishContentContext context, TPart instance) in \src\Orchard\ContentManagement\Handlers\ContentHandler.cs:line 164
   at Orchard.ContentManagement.Handlers.StorageFilterBase`1.Orchard.ContentManagement.Handlers.IContentStorageFilter.Published(PublishContentContext context) in \src\Orchard\ContentManagement\Handlers\StorageFilterBase.cs:line 87
   at Orchard.ContentManagement.Handlers.ContentHandler.Orchard.ContentManagement.Handlers.IContentHandler.Published(PublishContentContext context) in \src\Orchard\ContentManagement\Handlers\ContentHandler.cs:line 288
   at Orchard.ContentManagement.DefaultContentManager.<>c__DisplayClass42.<Publish>b__40(IContentHandler handler) in \src\Orchard\ContentManagement\DefaultContentManager.cs:line 386
   at Orchard.InvokeExtensions.Invoke[TEvents](IEnumerable`1 events, Action`1 dispatch, ILogger logger) in \src\Orchard\InvokeExtensions.cs:line 17
@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 10, 2014

@Piedone commented:

Yeah, Sebastien pinpointed this and fixed it. Would have been trivial to avoid but I missed it.

And this fix was sponsored by our client, CarolinaMLS. Carolina's site contains tens of thousands of content items so this fix was badly needed.

orchardbot commented Sep 10, 2014

@Piedone commented:

Yeah, Sebastien pinpointed this and fixed it. Would have been trivial to avoid but I missed it.

And this fix was sponsored by our client, CarolinaMLS. Carolina's site contains tens of thousands of content items so this fix was badly needed.

@orchardbot

This comment has been minimized.

Show comment
Hide comment
@orchardbot

orchardbot Sep 11, 2014

@StanleyGoldman commented:

Works for me, thanks guys!

orchardbot commented Sep 11, 2014

@StanleyGoldman commented:

Works for me, thanks guys!

@orchardbot orchardbot added this to the Orchard 1.8.x milestone Apr 13, 2015

@orchardbot orchardbot closed this Apr 13, 2015

@sebastienros sebastienros modified the milestones: Orchard 1.8.x, Orchard 1.8.2 May 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment