#30563: Optimize django.forms.widgets.Media.__add__.
--------------------------------------+------------------------------------
Reporter: David Dorothy | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: media | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by David Dorothy):
I apologize for my delay in responding to this. I had to switch tasks for
a little while to a different project that took a bit longer than
intended.
I have done some further research into this issue and have some
information based on the actual use. I have 194016 Media objects that are
being added together. For the JavaScript references when the lists are
de-duplicated in the way you describe above (in `__add__` not in `merge`)
the list is trimmed to 13 records. CSS is handled differently and I'm not
sure it is correct (see code later in this comment). This provides a
performance on my machine of around 3-5 seconds. This is not as fast as my
original optimization but is definitely close enough.
As far as sharing code, I cannot give you access to my repository.
However, I can show you the code where the adding of Media objects is
occurring at.
First, here is my attempt at improving the `Media.__add__` method:
{{{#!python
def combine_css(destination, css_list):
for x in filter(None, css_list):
for key in x.keys():
if key not in destination:
destination[key] = OrderedSet()
for item in x[key]:
if item:
destination[key].add(item)
class ImprovedMedia(forms.Media):
def __add__(self, other):
combined = ImprovedMedia()
combined._css_lists = list(filter(None, self._css_lists +
other._css_lists))
css_lists = {}
combine_css(css_lists, self._css_lists)
combine_css(css_lists, other._css_lists)
combined._css_lists = [css_lists]
combined._js_lists = list(OrderedSet([tuple(x) for x in
filter(None, self._js_lists + other._js_lists)]))
return combined
}}}
I am concerned that my `combine_css()` function may not be the best
implementation because it does keep order but it always results in exactly
one new `self._css_lists` which may not be the correct solution. I don't
think I have enough context to know for sure if this is right. Also, I
have some reservations about how I use `OrderedSet` for `Media._css_lists`
because in my initial attempts on the `Media._js_lists` I had to convert
the `OrderedSet` back into a `list` to make things work.
Second, here is the code where the `__add__` method is called:
{{{#!python
class StructuredStreamBlock(StreamBlock):
js_classes = {}
# ...
def all_media(self):
media = ImprovedMedia()
all_blocks = self.all_blocks()
for block in all_blocks:
media += block.
return media
# ...
}}}
The original code can be found in wagtail's source code here:
https://github.com/wagtail/wagtail/blob/e1d3390a1f62ae4b30c0ef38e7cfa5070d8565dc/wagtail/core/blocks/base.py#L74
Unfortunately I could not think of a way to monkey patch the Media class
in Django without actually changing the Django code in my virtual
environment.
Should I attempt at this point to create a fork and a branch (and
hopefully eventually a pull request) and see if the change I make break
any existing tests or should some others with more familiarity weigh in on
this first?
--
Ticket URL: <https://code.djangoproject.com/ticket/30563#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.f0e95627feb7779ca573c5730aad8121%40djangoproject.com.
