CMSPlugin.is_last_in_placeholder #1302

Closed
Schwankenson opened this Issue Jun 16, 2012 · 6 comments

Comments

Projects
None yet
4 participants
@Schwankenson

The function

def is_last_in_placeholder(self):
    """
    WARNING: this is a rather expensive call compared to is_first_in_placeholder!
    """
    return self.placeholder.cmsplugin_set.all().order_by('-position')[0].pk == self.pk

from cms.models.CMSPlugin returns true, if a plugin is the last Placeholder in a plugin. But it counts all Plugins in the placeholder, even the inline plugins.

I dont think, this is what somebody who calls the function expects (I didnt).

Maybe it would be a good idea, to pass a countinline=false parameter to the function...

@twoolie

This comment has been minimized.

Show comment
Hide comment
@twoolie

twoolie Jun 27, 2012

Contributor

A Great example of the perils of leaning on the ORM too much.

Firstly, this function will fill it's cache with all attributes of every plugin in that placeholder, then it will check just the pk of the first object. I really hope this function isn't used by anyone.

A better solution would be

return self.position == self.placeholder.cmsplugin_set.all().aggregate(Max('position'))['max_position']

That way, the database can calculate the result on it's side (better queryplan), reduce data transfered over network, and get the answer quicker.

Contributor

twoolie commented Jun 27, 2012

A Great example of the perils of leaning on the ORM too much.

Firstly, this function will fill it's cache with all attributes of every plugin in that placeholder, then it will check just the pk of the first object. I really hope this function isn't used by anyone.

A better solution would be

return self.position == self.placeholder.cmsplugin_set.all().aggregate(Max('position'))['max_position']

That way, the database can calculate the result on it's side (better queryplan), reduce data transfered over network, and get the answer quicker.

czpython added a commit to czpython/django-cms that referenced this issue Mar 19, 2013

@ojii

This comment has been minimized.

Show comment
Hide comment
@ojii

ojii Mar 19, 2013

Collaborator

fixed in e6402af

Collaborator

ojii commented Mar 19, 2013

fixed in e6402af

@ojii ojii closed this Mar 19, 2013

@twoolie

This comment has been minimized.

Show comment
Hide comment
@twoolie

twoolie Mar 20, 2013

Contributor

@czpython Your fix does nothing to address the terrible, terrible O(n^2) performance of this function.

Please consider changing the line to read either

# this will be faster if position field has an index
return self.pk == self.placeholder.cmsplugin_set.filter(parent__isnull=True).order_by('-position').only('id')[:1][0].pk

OR

# this will be faster if position field does not have an index
return self.position == self.placeholder.cmsplugin_set.filter(parent__isnull=True).aggregate(Max('position'))['max_position']
Contributor

twoolie commented Mar 20, 2013

@czpython Your fix does nothing to address the terrible, terrible O(n^2) performance of this function.

Please consider changing the line to read either

# this will be faster if position field has an index
return self.pk == self.placeholder.cmsplugin_set.filter(parent__isnull=True).order_by('-position').only('id')[:1][0].pk

OR

# this will be faster if position field does not have an index
return self.position == self.placeholder.cmsplugin_set.filter(parent__isnull=True).aggregate(Max('position'))['max_position']
@czpython

This comment has been minimized.

Show comment
Hide comment
@czpython

czpython Mar 20, 2013

Member

I don't understand how this is O(n^2), it's a single database query using indexes. Please profile it and show the results.

Member

czpython commented Mar 20, 2013

I don't understand how this is O(n^2), it's a single database query using indexes. Please profile it and show the results.

@ojii

This comment has been minimized.

Show comment
Hide comment
@ojii

ojii Mar 20, 2013

Collaborator

As @czpython said, this isn't O(n^2). Also the method clearly states in it's docstring that it's an expensive operation and further this is an undocumented API. As such, I'm more than happy with the patch that fixed the bug.

If you want to propose a more efficient way of solving this, please open a new ticket.

Collaborator

ojii commented Mar 20, 2013

As @czpython said, this isn't O(n^2). Also the method clearly states in it's docstring that it's an expensive operation and further this is an undocumented API. As such, I'm more than happy with the patch that fixed the bug.

If you want to propose a more efficient way of solving this, please open a new ticket.

@twoolie

This comment has been minimized.

Show comment
Hide comment
@twoolie

twoolie Mar 22, 2013

Contributor

@czpython You are right, it is not on it's own O( n^2 ) but using it can be.

Consider an example of a page with a placeholder which has 10 plugins inside of it.
Each of these renders using the following template.

<div class="cms-plugin cms-plugin-exampleplugin {% if plugin.is_last_in_placeholder %}last{% endif %}" >
  ...... Content .....

( I know that it's a completely synthetic worst case but where else would you use a function like this?)

Then for each of those 10 plugins on the page, you are calling this function which retrieves 10 rows from the database and throws away everything but 1 single value.

That makes 100 rows retrieved or ~O( n^2 ) performance hit just for trying to annotate whether we're last in the placeholder.

At least by attaching .only('id')[:1][0].pk or even better [:1].values('id')[0]['id'] to the end of the current solution your bringing it down to O(n) for the page rendering operation.

( I also know that I'm being anal retentive over nothing but I'm currently slogging through someone else's code that uses the simplest possible ORM calls and it's driving me insane how slow it makes everything when these functions with small performance penalties are then called in a loop. Death by a thousand cuts. And everything in cms is just so well optimised everywhere else. I'll just shut up about it now, I promise. )

Contributor

twoolie commented Mar 22, 2013

@czpython You are right, it is not on it's own O( n^2 ) but using it can be.

Consider an example of a page with a placeholder which has 10 plugins inside of it.
Each of these renders using the following template.

<div class="cms-plugin cms-plugin-exampleplugin {% if plugin.is_last_in_placeholder %}last{% endif %}" >
  ...... Content .....

( I know that it's a completely synthetic worst case but where else would you use a function like this?)

Then for each of those 10 plugins on the page, you are calling this function which retrieves 10 rows from the database and throws away everything but 1 single value.

That makes 100 rows retrieved or ~O( n^2 ) performance hit just for trying to annotate whether we're last in the placeholder.

At least by attaching .only('id')[:1][0].pk or even better [:1].values('id')[0]['id'] to the end of the current solution your bringing it down to O(n) for the page rendering operation.

( I also know that I'm being anal retentive over nothing but I'm currently slogging through someone else's code that uses the simplest possible ORM calls and it's driving me insane how slow it makes everything when these functions with small performance penalties are then called in a loop. Death by a thousand cuts. And everything in cms is just so well optimised everywhere else. I'll just shut up about it now, I promise. )

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