slow loading of media files

[clime]

Hey,

upgrading to 1.0 seems to cause slow downloading of media files into browser. They are loaded in batches of aprox. six images and there is always a long pause between loading of each batch. Here is screenshot from chrome developer tools:

With debug toolbar disabled, this doesn't happen and images are all loaded almost immediatelly.

I am always cleaning the browser cache between individual tries to get the results.

[clime]

If I disable Static files panel, the problem disappears. Strangely the static files themselves does not seem to be influenced if the panel is enabled. This is the code to serve media files:

if settings.DEBUG:
    urlpatterns += patterns('',
        (r'^media-files/(?P<path>.*)$', 'django.views.static.serve', {
        'document_root': settings.MEDIA_ROOT}))

[aaugustin]

@jezdez — what do you think?

[jezdez]

Huh, will look into this.

[ghost]

Same problem with Django 1.7b1 and latest django-debug-toolbar:

from django.conf.urls.static import static


if settings.DEBUG:
    urlpatterns += static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT)

[spookylukey]

I think I have tracked this down:

The slowness can be eliminated by removing the call to get_staticfiles_finders from StaticFilesPanel.process_response. The problem is just that this method takes a non-trivial amount of time to run when you have a lot of media files, for instance (3000+ on my current project).

A further problem is that this expensive method is run for every request, including ones that will never need it, because they don't return HTML - including requests for the media files themselves. So, for media heavy sites (where the total number of media files is large, and the number per page is likely to be large), you get hit on both counts.

Digging deeper, Panel.process_response is run for every panel, whether or not the response can have the toolbar inserted. This seems like a very unnecessary performance hit! The current design allows Panel.process_response to return its own response, but this is only used once, by the redirect panel.

So it seems that for most of the panels, the process_response method should be empty and there should be a generate_stats method instead, which is called only conditionally after DebugPanelMiddleware.process_response has decided that it is going to insert the toolbar.

[jezdez]

@spookylukey Nice digging! That makes sense (yikes, 3k files!), your proposed API changes make sense to me, too.

[danpaulson]

Bump! Running in to this on a rather large site, wondering if it's slated to be fixed any time soon?

[tim-schilling]

It probably won't be any time soon, but I'll try to get a solution together the next time I spend a large chunk of time on the project.

[tim-schilling]

@spookylukey When you said most of the panels' process_response methods should be empty, were you referring to the RedirectsPanel as the one exception? If so, I think we may want to change your design so that the current process_response only gets called when it's going to be inserted and then create a interrupt_response method that panels like RedirectsPanel need to be called before.

The reasoning is that if we change the purpose of process_response and use a different method to handle that logic, it will impact most of the third party panels. Since development on the toolbar is slow, I'd rather keep the API as stable as possible. Obviously any third party panels that return a response would still need to be updated and I think that's only https://github.com/joymax/django-dtpanel-htmltidy.

[spookylukey]

@tim-schilling it's exactly because of the compatibility problem with 3rd parties that I made the suggestion that way round. Existing Panel subclasses may assume that their process_response method gets called for every request (for example, something that is a drop-in replacement for the current RedirectsPanel). If you want to keep full compatibility with them (which you might not care about), you can't change it so that process_response is only called sometimes.

With my suggestion, those 3rd party panels wouldn't be affected - they would be using process_response for the wrong purpose, but they would still work. The upgrade notes would say "For best performance, if you are not returning an actual response in process_response, you should rename it to generate_stats" (or choose some better name).

How you want to handle this is entirely up to you, though.

[tim-schilling]

Ah, gotcha. That makes sense, thank you for clarifying!

[tim-schilling]

Can someone test out #731 to verify that it's improved the loading of media files before I merge it in?

[aaugustin]

Closing as #731 was merged.