Hi all,
IMHO this patch would still make django issue illogical signals in admin.
frans and I have pointed out that the __set__ method in the
ManyRelatedObjectsDescriptor is flawed[1-2], which is used by admin.
The __set__ method would save a many-to-many relationship by clearing all
entries in a field and then reinserting them back. This would work in the
old days when no many-to-many signal is around. However, with the addition
of this signal, all saving through admin would issue two signals: first, a
signal with action 'clear', and then, a signal with action 'add'. Signal
with remove action would never be correctly issued.
frans and I had provided some patches to work this out. However, I'm afraid
my last solution in [2] would incorrectly issue signal with 'clear' action.
A more appriate solution would be:
manager = self.__get__(instance)
previous=set(manager.all())
new=set(value)
if previous and not new: # correction against [2] here.
manager.clear()
else:
added = new - previous
removed = previous - new
if removed:
manager.remove(*removed)
if added:
manager.add(*added)
[1]: http://code.djangoproject.com/ticket/5390#comment:40
[2]: http://code.djangoproject.com/ticket/5390#comment:55
------------------------
Xia Kai(夏恺)
xia...@gmail.com
http://blog.xiaket.org
--------------------------------------------------
From: "Russell Keith-Magee" <freakboy3...@gmail.com>
Sent: Friday, January 08, 2010 8:04 PM
To: "Django Developers" <django-developers@googlegroups.com>
Subject: Call for comment: #5390 Many To Many signals
Hi all,
I've just uploaded a patch for #5390, which adds signals to
many-to-many modifying operations.
This is a ticket that was a low-priority accepted item for 1.1
(ORM-18), but got bumped due to time [1]. For some reason, it wasn't
resubmitted for 1.2.
The purpose of this post is twofold:
1) To establish if there are any objections to introducing this
signal. Historically, there has been a performance objection to adding
new signals, so I want to give some numbers of the performance impact
and see if anyone objects. and
2) To get feedback on the implementation.
Barring objections or major design changes, I'd like to commit this
early next week.
1. Performance
--------------
The last time signals came up [2], Jacob provided some helpful
baseline data on the speed of signals:
Doing Nothing: 0.00283 (0.000000028 per call)
Plain func call: 0.07054 (0.000000705 per call)
Signal; 0 handlers: 0.09331 (0.000000933 per call)
Signal; 1 handler : 0.79125 (0.000007912 per call)
Signal; 10 handlers: 3.36051 (0.000033605 per call)
Signal; 100 handlers: 27.56269 (0.000275627 per call)
* In Python, calling a function (``handle()``) takes 25 times as long
as doing nothing (``pass``).
* Dispatching a signal when there's no handlers is about 1.3 times a
function call.
* Dispatching a signal when there's one handler is about 11 times the
cost of a function call.
* Addition of additional receivers scales linearly.
The other (completely unscientific) test is the Django test suite
itself. I've run the test suite with and without the patch from #5390.
What I have observed is that the discrepancies in test runtime caused
by normal fluctuations in background system load are more significant
than the slowdown caused by adding m2m signals (that is - the vanilla
suite sometimes takes *longer* than the suite with the patch applied).
So - based on my this analysis, there is a performance impact, but it
isn't especially significant or noticeable.
2. Implementation
-----------------
The patch I have uploaded [3] introduces a single new signal -
m2m_changed. This decision is based on design feedback from Malcolm
[4].
The signal is attached to the m2m through class (the automatically
generated m2m class that is the result of the m2m refactor). This
class is the 'sender' of the signal.
The signal handler is provided with the sender, as well as:
* The instance that contains the m2m field that is being modified
* A string tag indicating if this is an "add", "remove" or "clear" call
* A boolean indicating whether this is the 'forward' or 'reverse'
relation.
* The class of model that is being added/removed/cleared
* The primary keys of the instances that are being added to the
relation (or None in the case of a call to clear the relation)
The docs and tests on the patch [3] give a more detailed description
and examples of the operation of the signal.
Any feedback on this design?
[1] http://code.djangoproject.com/wiki/Version1.1Features
[2]
http://groups.google.com/group/django-developers/browse_thread/thread/5a39839e2aa8df53
[3]
http://code.djangoproject.com/attachment/ticket/5390/t5390-r12120.1.diff
[4]
http://groups.google.com/group/django-developers/browse_thread/thread/afe6ad7994d868ba
Yours,
Russ Magee %-)
--
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to django-develop...@googlegroups.com.
To unsubscribe from this group, send email to
django-developers+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en.
