Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance issues with get_available_proceedings function #36

Closed
psychok7 opened this issue Jan 23, 2017 · 19 comments
Closed

Performance issues with get_available_proceedings function #36

psychok7 opened this issue Jan 23, 2017 · 19 comments

Comments

@psychok7
Copy link
Contributor

@psychok7 psychok7 commented Jan 23, 2017

@javrasya there seems to be a performance issue with .get_available_proceedings function.

So, i followed your youtube admin tutorial and in the beginning everything seemed to work fine.

Now i have been noticing a performance issue with just 15 objects in my model. When digging a bit further, i realized the problem was in the get_available_proceedings method:

    def river_actions(self, obj):
        content = ""
        for proceeding in obj.get_available_proceedings(self.user):
            content += create_river_button(obj, proceeding)
        return content

If i leave it like this, my page in the admin takes about 6 seconds to load (with just 15 objects). If i comment the function, it loads in less than 1 second.

My model looks like this:

class QRequest(models.Model):
    # Some fields and:
    status = StateField(editable=False, verbose_name=_('status'))

Any ideas what could be causing this in that function and how it can be fixed?

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Jan 24, 2017

Let me reproduce it, what db you use?

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Jan 24, 2017

@javrasya I am using Postgresql 9.5 + Django 1.10.x

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Jan 27, 2017

@javrasya any ideas about this??

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Jan 29, 2017

@psychok7 I am so sorry for late response. I can't find time for django-river nowadays unfortunately and that makes me sad. I will try to spend more time about django-river after today. Because it seems it is needed.

I tried to add a test case for that which is with SQLite, will commit it soon. But, couldn't reproduce it.

I will try to reproduce it psql 9.5 tomorrow. I will be informing you.

Btw, As you may have seen the logic in the function you talked about, it all lay on DB operations. I don't like it. This is probably the problem makes slowness. I think we can handle the logic in the language itself(python). I think this will be the way of much faster than that. So I will try to reimplement it by doing the same thing in python.

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Jan 30, 2017

@psychok7, I could produce something different with Postgresql 9.5.4 , with 100 object records, With my scenario, it took 700 ms which is not that much. I am testing it on my local environment and I have SSD on my computer.

Could you please share your river app dump output with me;

python manage.py dumpdata  --indent=2 > db_dump.json

So, I can try that with your scenario.

javrasya added a commit that referenced this issue Jan 30, 2017
javrasya added a commit that referenced this issue Jan 30, 2017
@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Jan 30, 2017

@javrasya yeah sure i understand. I will try to help out with testing and a few lines of code whenever i can as well.

Unfortunately i cannot share the all database because it's production and confidential information, but the model is pretty simple:

class QRequest(models.Model):
    r_id = models.CharField(
        _('r_id'), max_length=50, null=True, blank=True)
    name = models.CharField(_('name'), max_length=200)
    phone = models.CharField(_('phone'), max_length=50)
    email = models.EmailField(verbose_name=_('email'))
    
    typology = models.CharField(
        verbose_name=_('typology'), max_length=1, choices=TYPOLOGY_CHOICES,
        default='a')

    status = StateField(editable=False, verbose_name=_('status'))

    created_on = models.DateTimeField(
        auto_now_add=True, verbose_name=_('created_on'))
    updated_on = models.DateTimeField(
        auto_now=True, verbose_name=_('updated_on'))

    class Meta:
        verbose_name = _("qrequest")
        verbose_name_plural = _("qrequests")
        permissions = (
            ('view_qrequest', 'Can view QRequest'),
        )

I also have an SSD and i7 and with a normal model its usually pretty fast. I am using Python3 and docker if that helps.

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Jan 30, 2017

@psychok7 So, can you just share river app DB dump then, Is that also confidential? I need to create the same scenario as yours as much as possible.

I mean;

python manage.py dumpdata river  --indent=2 > river_dump.json
@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Jan 30, 2017

@javrasya oh that i can share. it in the attachment

river_dump.json.zip

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Feb 6, 2017

@javrasya have you had any progress with this issue??

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Feb 6, 2017

@psychok7 I couldn't reproduce it. I tested it with SQLite and PostreSQL 9.5.4. You can also test it by changing the database configuration in test_settings.py. The PostgreSQL one is commented out. It seems you have another problem.

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Feb 6, 2017

@javrasya ok. Anyways are you still planning on converting the logic into python and avoid the DB hits?

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Feb 6, 2017

@psychok7 Yes, I do.

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Feb 6, 2017

@javrasya ok. hopefully that will be enough to fix my problem.

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Feb 14, 2017

@javrasya hi again.

i have some more information on this issue. so while playing around i found that my performance issue is related to passing a user to obj.get_available_proceedings(user=self.user). In other words if i just obj.get_available_proceedings() it loads normally.
Maybe this is related to the groups i have (maybe that's why you cannot replicate my issue).

Do you see anything wrong with the method regarding the user groups?

If not i believe one workaround for me would be just not to use river proceeding meta groups and eventually protect it manually but that does not look like the best solution

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Feb 19, 2017

hi @javrasya , any thoughts on my last comment?

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Feb 21, 2017

@psychok7 I tried to do it in a bit more pythonic way. Authorization part was still hitting DB. I saw that the current version is faster than the way of doing it in pythonic. I test it with 1K objects with your scenarios. But as I said, authorization was hitting DB on that test.

I will look at that group authorization phase that you mentioned and try to understand what is your problem.

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Aug 7, 2017

@psychok7 are you still having this problem? Is there any further update on this one? If not, we can close this.

@psychok7

This comment has been minimized.

Copy link
Contributor Author

@psychok7 psychok7 commented Aug 7, 2017

@javrasya i think this is still happening because it was not addressed

@javrasya

This comment has been minimized.

Copy link
Owner

@javrasya javrasya commented Jan 5, 2019

Couldn't reproduce, so that I am closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.