subject:"Re\: \[Django\] #30563\: Optimize django.forms.widgets.Media.__add_

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2020-09-24 Thread Django

#30563: Optimize django.forms.widgets.Media.__add__.
-+-
 Reporter:  David Dorothy|Owner:  David
 Type:   |  Smith
  Cleanup/optimization   |   Status:  assigned
Component:  Forms|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  media| Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"1ac337cca61570ed64acf02e1700483f7fd02cf6" 1ac337cc]:
 {{{
 #!CommitTicketReference repository=""
 revision="1ac337cca61570ed64acf02e1700483f7fd02cf6"
 Refs #30563 -- Added tests for merging form Media with different ordering.
 }}}

-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.15514742a145d3bdd44799af1eeb4ebc%40djangoproject.com.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2020-09-24 Thread Django

#30563: Optimize django.forms.widgets.Media.__add__.
-+-
 Reporter:  David Dorothy|Owner:  David
 Type:   |  Smith
  Cleanup/optimization   |   Status:  assigned
Component:  Forms|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  media| Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by felixxm):

 * stage:  Accepted => Ready for checkin


-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.e29ad516c09cfe4a7f029a70b80ac43e%40djangoproject.com.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2020-08-31 Thread Django

#30563: Optimize django.forms.widgets.Media.__add__.
-+-
 Reporter:  David Dorothy|Owner:  David
 Type:   |  Smith
  Cleanup/optimization   |   Status:  assigned
Component:  Forms|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:  media| Triage Stage:  Accepted
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by David Smith):

 * owner:  nobody => David Smith
 * status:  new => assigned
 * has_patch:  0 => 1


-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.ad9c4048e7ddd1885b5cd1e654cec62c%40djangoproject.com.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2019-10-10 Thread Django

#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: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.f0e95627feb7779ca573c5730aad8121%40djangoproject.com.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2019-06-28 Thread Django

Comment (by David Dorothy):

Replying to [comment:2 Matthias Kestenholz]:
> I wonder how many distinct lists of assets you have? In the example
above you're merging the same list 100'000 times.
>

I encountered this issue when using the StreamField from Wagtail
(http://wagtail.io) where we have a use case that dictates creating
significant nesting of common forms to be able to edit the stream of
output. To be fair, our use case of Wagtail is beyond the expected use of
Wagtail as the performance is poor. I have created a customization that
helps prevent the performance issue in Wagtail by declaring all stream
objects as classes that only generate their forms once per object. This
workaround resolves the wagtail performance issue (for our needs) but not
the `Media.__add__` performance issue in Django.

I may be able to share my code if that will help. I do not personally own
that code though so I'd have to get permission.

We have nested classes of up to four or five levels deep and about 70
unique classes. But because of the way it works the number of forms
generated grows exponentially. Since these are all the same classes though
the number of distinct lists is limited that I think your de-duplication
idea would probably work.

I'm not sure whether your code works or not because I actually short
circuited in a subclass of a wagtail class to overcome the issue. I might
be able to replace the Django Media class via monkey patching with a
different version to test but I'm not sure. If I get some time to try this
out I'll post a followup comment and try to include some sample code or
possibly a patch with some matching tests. Not sure how much time I will
be able to dedicate to this.

Thanks for looking into this issue.

--
Ticket URL:
Django
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.61ed47ab91c652a00030351fdbc51553%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2019-06-20 Thread Django

#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 jun0jang):

 j

-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.94d9ebee627783234f6e1edea193aaea%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

2019-06-12 Thread Django

#30563: Optimize django.forms.widgets.Media.__add__.
--+
 Reporter:  didorothy |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 Matthias Kestenholz):

 I wonder how many distinct lists of assets you have? In the example above
 you're merging the same list 100'000 times.

 When calling `merge` earlier than at the end it will always be
 (relatively) easy to construct a failing test case.

 Maybe deduplicating the list of assets to merge would help? Something like
 (completely untested)

 {{{
 ~/Projects/django$ git diff
 diff --git a/django/forms/widgets.py b/django/forms/widgets.py
 index c8ec3c35d5..248c29b4c1 100644
 --- a/django/forms/widgets.py
 +++ b/django/forms/widgets.py
 @@ -124,7 +124,7 @@ class Media:
  """
  dependency_graph = defaultdict(set)
  all_items = OrderedSet()
 -for list_ in filter(None, lists):
 +for list_ in OrderedSet(filter(None, lists)):
  head = list_[0]
  # The first items depend on nothing but have to be part of
 the
  # dependency graph to be included in the result.
 ~/Projects/django$
 }}}

 It might be better for the memory usage to deduplicate earlier (inside
 `Media.__add__` maybe).

-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.860b693d7da2d272bb998e282173318a%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add. (was: django.forms.widgets.Media.add Performance Issue)

2019-06-12 Thread Django

#30563: Optimize django.forms.widgets.Media.__add__.
--+
 Reporter:  didorothy |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
--+
Changes (by felixxm):

 * keywords:  performance, unintended consequences => media
 * cc: Matthias Kestenholz (added)
 * stage:  Unreviewed => Accepted


Comment:

 I confirmed that 959d0c078a1c903cd1e4850932be77c4f0d2294d caused a
 performance issue. We should be able to optimize this by calling merge
 more often.

-- 
Ticket URL: 
Django 
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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/067.2fe493f564952a6b2f772fc7e3f0fcfe%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add.

Re: [Django] #30563: Optimize django.forms.widgets.Media.add. (was: django.forms.widgets.Media.add Performance Issue)

8 matches

Site Navigation

Mail list logo

Footer information