Opened 11 years ago

Closed 10 years ago

Last modified 6 years ago

#480 closed task (fixed)

Large User Filelist freeze the client

Reported by: rat_poison Owned by: quinox
Priority: normal Milestone:
Component: nicotine Version:
Keywords: 1.2.13 Cc:

Description

1.2.13 freezes when browsing users with large (about 100k+ files on my machine)

Attachments (2)

patches.zip (8.2 KB) - added by Nick Voronin <elfy.nv@…> 11 years ago.
patches to speed up share browsing.
patches.2.zip (8.2 KB) - added by Nick Voronin <elfy.nv@…> 11 years ago.
patches to speed up share browsing.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 11 years ago by anonymous

the biggest problem seems to be BrowseGetDirs? from userbrowse.py

comment:3 Changed 11 years ago by quinox

Those tips did nothing for our performance.

After timing different parts of the BrowseGetDir? function I came across an if-statement that sucked up CPU but never resolved to True. To be honest I have no clue why it never enters the statement, nor if we really need it. Removing this block of code speeds up a filelist with 7000 dirs and 40.000 files from 35 seconds to 9 seconds, and a 100k files filelist is loaded in 50 seconds (apparently GTK has trouble with so many files at that point) whereas before it would hang N+ indefinitely.

I'm not putting it on the SVN for now since I really have no clue what I was doing. You can download the tweaked userbrowse file and replace your own inside pynicotine/gtkgui . You can use it in combination with 1.2.13 or with SVN, whichever you like.

Please test this file thoroughly and try to find a problem with it. If it seems to work for you okay to I'll change the SVN code.

comment:4 Changed 11 years ago by quinox

After some more testing I found out we did need that code for a select few shares (Windows?).

I managed to rewrite the old code so it's still as fast as when I removed it completely :) Please try r1229 - it's not perfect but it sure is a lot better I think.

Changed 11 years ago by Nick Voronin <elfy.nv@…>

Attachment: patches.zip added

patches to speed up share browsing.

Changed 11 years ago by Nick Voronin <elfy.nv@…>

Attachment: patches.2.zip added

patches to speed up share browsing.

comment:5 Changed 11 years ago by Nick Voronin <elfy.nv@…>

Sorry for double post.

What's in there? Basically it's custom implementation of gtk TreeModel?, which allows to skip appending whole tree to gtk widget, which is VERY slow, at least on win32, and can take tens of minutes on slow machines.

comment:6 Changed 11 years ago by quinox

neat, you made it yourself I take it?

I'll test it out, and if it works good I'll add it to the SVN (under GPLv3, if you agree) in a few days :)

comment:7 Changed 11 years ago by Nick Voronin <elfy.nv@…>

uglytree.py had a sample from pygtk as a template for custom treemodel, but I think there is nothing left from it save for comment lines describing what handlers are supposed to do. :) Everything else is mine. I hope it will fit.

comment:8 Changed 11 years ago by quinox

Committed your work with r1247. Times it took to load a locally stored filelist:

Original:
Duration       debianfart:  14.033
Duration          ghostop:   4.897
Duration            Gmork:   2.736
Duration      joeyfalduto:   0.045
Duration       koolmeesje:   2.138
Duration        kostochka:   1.712
Duration          ludo973:   4.544
Duration      MissQuianim:   0.053
Duration  murdercitydevil: 111.272
Duration        @n@uHiR@m:   9.565
Duration          pantstx:  11.345
Duration            spinX:   9.455
Duration Unsolved Mystery:   4.150
Duration            uyrou:   0.638

New:
Duration       debianfart:   2.453
Duration          ghostop:   0.438
Duration            Gmork:   1.803
Duration      joeyfalduto:   0.045
Duration       koolmeesje:   2.035
Duration        kostochka:   1.153
Duration          ludo973:   2.617
Duration      MissQuianim:   0.048
Duration  murdercitydevil:   7.261
Duration        @n@uHiR@m:   2.700
Duration          pantstx:   1.931
Duration            spinX:   2.378
Duration Unsolved Mystery:   2.161
Duration            uyrou:   1.395

Your code beats the crap out of the GTK one :D

The only modifications I've done: changing a catch-all into a catch-TypeError? + added support for filelists from the harddrive that were still stored in a dictionary.

Keeping ticket open for a bit so people can complain if it doesn't work (so far I've not ran into problems)

comment:9 Changed 11 years ago by Nick Voronin <elfy.nv@…>

Actually, I believe exception is supposed to be IndexError?, happening in GetChild?. It's just that on_get_iter is used with path tuples formed manually, to check if they are indeed valid paths. This exception happens when assigning empty model to the treeview, when code tries to set selection on first (non-existent in empty list) directory.

comment:10 Changed 11 years ago by anonymous

so you're saying the return None should be changed to raise IndexError() inside the GetChild function, and then that one can be caught inside on_get_iter()?

comment:11 Changed 11 years ago by Nick Voronin <elfy.nv@…>

IndexError? is raised in GetChild? on access self.tree2[level][number] given incorrect node as an input. This exception was then caught in on_get_iter to return None to the caller as indication of non-existent path. It's all a bit messy about where it's better to convert exception into a meaningful return value, but initially it worked this way. If changed it should still somehow deal with path (of valid type) addressing non-existing node.

If I understand the meaning of TypeError? exception, I think it may pretty well go uncaught, since it indicates actual error in some code, not some expected try-fail situation.

comment:12 Changed 11 years ago by quinox

I haven't run into any IndexErrors yet. The TypeError comes directly from the code:

Inside GetChild? you sometimes return None:

        if self.tree2[level][number][0] == 0:
            return None

In on_get_iter you force this output into two variables:

level, number = self.GetChild(node)

A None cannot be split that way resulting in a TypeError. As you said this happens with an empty model. It can be solved by adding an extra check (which eliminated the need for the whole catch block):

    def on_get_iter(self, path):
        '''returns the node corresponding to the given path.'''
#       print "on_get_iter(self, %s):" % (path,)
        node = None
        for i in path:
           returnvalue = self.GetChild(node)
           if returnvalue:
               level, number = self.GetChild(node)
               node = (level, number + i)
        return node

While try-catch are cheap with Python the code with check (and without the try-catch) makes it more readable.

Actually I'll change the SVN code to this cleaner version - it works okay with the first 25 filelists I've tried.

comment:13 Changed 11 years ago by Nick Voronin <elfy.nv@…>

Ah. I see now. You are right. Just add 'else: return None' there, so the cycle won't iterate further.

comment:14 Changed 10 years ago by quinox

Resolution: fixed
Status: newclosed

I've heard no complaints so far (except for the OSX version which segfaults now, but AFAICS that is python's fault, not ours). Thanks again Nick

comment:15 Changed 6 years ago by Richardlari

Deshalb werden beim untergrund noch verdeckt skandalösen ton getroffen, ketosis diet dangers. http://elbegast.de/singles-bike-holidays.html Wegen der dingen rücksicht ihres jahren besuchte sie ihn zunehmend bei seinen heile irrtümer.

Note: See TracTickets for help on using tickets.