Behavior changes

[kovidgoyal]

I've started work on a new, multi-threaded (among many other new features) database backend for calibre. Part of making that happen is ensuring that all file I/O on files in the library happens in the database class, so that it can protect the I/O appropriately.

I've committed changes to the format() and cover() functions in the current db class to make that happen. Basically, these functions used to originally pass back references to the files in the library. Now they copy the file to a temp file and pass back a reference to it, instead.

These changes have obvious performance implications, since they mean that every file has to be copied, so review your code to ensure that the changes don't have serious ill effects. In many cases, by using a different code structure, the performance impact can be mitigated.

Also, the format_abspath function has been deprecated and will no longer work reliably in the future, so if your code uses it, change the code to use format(..., as_path=True) instead (this is only available in current source).

I've already made the necessary changes to all the code in the calibre source tree, so this is really a heads up for plugin developers.

feel free to ask if you have any questions.

[kiwidude]

I am not at my pc right now to review your changes and what I call but I would guess my open with plugin uses both of those, and it intentionally wants to work with write access to the original files in place.

Suggestions? Edit in Sigil and resizing covers are continual tasks I and many other users take advantage of. I would hate to have to put a tweak ePub type of dialog up where the user is blocked from doing other actions in calibre while editing.

[kiwidude]

I did a search for format_abspath and found I have six plugins which use that alone (Count Pages, Extract ISBN, Find Duplicates, Modify ePub, Open With and Quality Check). It is at times like this I greatly regret writing so many plugins.

I'm going to need to as you say look at all of these on a case by case basis, but undoubtedly it is going to dramatically slow down many plugins if I just roll with it. Some of these plugins like Count Pages, Extract ISBN and Modify ePub already make their own copies when they queue up for the background jobs, so I don't want to be making "copies of copies". Plugins like Quality Check and Find Duplicates effectively ran modally, so it didn't need to copy the files because nothing else was allowed access to them from the GUI. Open With as I mentioned above directly worked with the files, so that the user wasn't prevented from doing actions in Calibre while you edited externally, and didn't have to click on another dialog when done.

Ugh. This is not a trivial change.

[theducks]

@Kiwidude
Thanks for checking.
I routinely open more than 1 book (of a series) in Sigil to borrow 'stylistic' elements, use 'Tweak EPUB' to rip objects for use. and Open Different in the Calibre-Viewer formats: 'All open at the same time'.
I see this as possibly having an impact on performance

[chaley]

@Kovid: I imagine that this is a step in the direction we were discussing a year ago when designing the new metadata interface: making possible non-local data stores? If so, it makes sense, as cloud-based libraries won't support path-style access.

[kiwidude]

Yeah, like you I sometimes do a Tweak ePub and Edit in Sigil at the same time (using Edit in Sigil to get easy visual access to the styles, and Tweak ePub to actually make the changes if it has an NCX I want preserved). Or I might start editing a document in Sigil and find it is a massive amount of work, but I leave Sigil open in the background and come back to it in stages, working independently (and restarting) Calibre in the interim.

I don't know what options if any I have around this. If I understand correctly Kovid is trying to make it "safe" for anything to read/write to support multiple access. Which in the long term will be great. The downside as I see it (without poring through the code) is that for everyone who doesn't do "multiple process" stuff, you still have to take the hit.

I'll wait to hear from Kovid before I push the panic button on this, but to be honest right now I am feeling rather ill for a couple of the plugins (and that I have less than two days before the next Calibre release goes out). I'm feeling a bit like roadkill at the moment.

[theducks]

Originally Posted by kiwidude

Yeah, like you I sometimes do a Tweak ePub and Edit in Sigil at the same time (using Edit in Sigil to get easy visual access to the styles, and Tweak ePub to actually make the changes if it has an NCX I want preserved). Or I might start editing a document in Sigil and find it is a massive amount of work, but I leave Sigil open in the background and come back to it in stages, working independently (and restarting) Calibre in the interim.

I don't know what options if any I have around this. If I understand correctly Kovid is trying to make it "safe" for anything to read/write to support multiple access. Which in the long term will be great. The downside as I see it (without poring through the code) is that for everyone who doesn't do "multiple process" stuff, you still have to take the hit.

I'll wait to hear from Kovid before I push the panic button on this, but to be honest right now I am feeling rather ill for a couple of the plugins (and that I have less than two days before the next Calibre release goes out). I'm feeling a bit like roadkill at the moment.

I wonder if building a "Lock" table and folder file pair.
any operation that would affect the folder/contents tests and places a appropriate Lock -entry.
Any non-originator Lock prevents a title rename
Other locks: Cover edit, conversion (from: to format locks

Global: Author name (Title Parent folder) rename would be blocked on any contained title lock.

In the case of your 'Open With' , it would either need a Lock control setting, or a simple 'Sorry, Title/Author In Use by others' Block

[kovidgoyal]

@kiwidude: First, format_abspath remains in place until the new db backend is completed, so you dont have to do anything right away. All you need to do right away, is check uses of cover() and format(). Replace the former with abspath(id)+'/cover.jpg' and the latter with format_abspath.

Second, if your plugins already make copies there is no need to make copies of copies, simply use db.copy_format_to to make your first copy. Or use the path returned by db.format(..., as_path=True) as your copy.

When it comes to opening files in external editors, I don't really see a solution short of launching the external process with a temp file and then watching the file in a thread. Whenever the file changes, you update it in the calibre library. If you like, I can help you by adding code to implement cross platform file change notification in the calibre core.

@chaley: Yes, that's the idea, it also makes the library store much more robust, since it prevents multiple processes/threads from writing to the same file at once (or in windows throwing an error).

[kiwidude]

Thks for the response Kovid, and confirming format_abspath isn't disappearing quite yet to give me time to rethink things...

Luckily I don't have any usages of cover() and format(), though that doesn't mean anything in terms of the bigger scheme of things.

Here are the issues I have that come to mind currently:
(1) Modify ePub and Count Pages - change to use db.copy_format_to() as you suggest. These are the "easy" ones relatively speaking though it requires a bit of refactoring since I currently pass paths around. Incidentally I wouldn't want to use the copy from db.format() because I would presume I have no control of the lifetime of the directory in that situation. Waiting until Calibre exits is "too long", and db.format() gives you no control over the directory.

(2) Extract ISBN - I was wrong about this plugin in that it doesn't currently copy files since it can end up scanning every format for a book. I shall have to bite the bullet and copy all files for a book as each book is processed I guess.

(3) Find Duplicates - this is a real problem for the binary comparison. Copying files just so you can compute a hash on them? Performance would go to the toilet - remember this is comparing every single format of every book in your library, and it makes use of the file timestamps to decide whether to recompare. Welcome any suggestions on this.

(4) Quality Check - the Check ePub functions in this are likewise a major problem. As once again every single ePub in your library is going to have to be copied every time you run a check. Again no ideas on how to resolve this currently

(5) Open With - if what you suggest of watching a temporary file with a thread is the only solution then it has to be so (in which case your help is much appreciated). Though should you close Calibre while having the app open that will have some implications though (temp files not being cleaned up due to file locks, users losing changes etc).

[kovidgoyal]

1) The temp file will last as long as calibre is open and it is unique (i.e. you own it) so you can delete it whenever you like.

3) Use as_file=True when calling format(). This copies the file into a SpooledTemporaryFile (which keep the entire contents of the file in memory unless the file is larger than 30MB in which case it is copied). Or I can add a method to db to compute hashes, if you send me an implementation of the method. Either is fine by me.

4) Does it really matter? Use as_file again. The vast majority of epubs are well under 30MB. This is assuming quality check runs in the main process. If it runs in a sub process, then you can change it to have two stages, one which runs in the main process and copies the files out and the second which unzips them and does the checking. This is how save to disk/bulk convert work in calibre.

5) I'm not implying that this is the only solution, just that it is the only solution I can think of. If you have other suggestions, I am happy to hear them.

Remember that most ebook files are a few MB and copying those around on a disk takes well under a second.

[chaley]

Originally Posted by kiwidude

(3) Find Duplicates - this is a real problem for the binary comparison. Copying files just so you can compute a hash on them? Performance would go to the toilet - remember this is comparing every single format of every book in your library, and it makes use of the file timestamps to decide whether to recompare. Welcome any suggestions on this.

I suggest that computing the hash be added to the API. No need to copy. As FD is eventually to go to calibre core, this may be the right approach.

Another suggestion: return a pipe instead of a temp file. The file must be read anyway, so reading a pipe shouldn't be more costly then reading the file. DB2 equivalent would need to handle filling the pipe without blocking, but there are several ways to do that. The easiest is to use a thread.

4) The pipe solution would work here as well.

[kovidgoyal]

@chaley: A pipe sounds interesting, I've never implemented one, I guess it is basically a FIFO Queue that only reads a few KB at a time into the queue. The question is what is the best architecture for filling the buffer? A separate thread or just fill when emptied in the emptying thread? The tradeoff, I guess, is between amount of time file handle has to be help open vs. efficiency?


This might work as a good alternative to the current use of a SpooledTempFile.

[chaley]

Originally Posted by kovidgoyal

@chaley: A pipe sounds interesting, I've never implemented one, I guess it is basically a FIFO Queue that only reads a few KB at a time into the queue. The question is what is the best architecture for filling the buffer? A separate thread or just fill when emptied in the emptying thread? The tradeoff, I guess, is between amount of time file handle has to be help open vs. efficiency?

Pipes are indeed fifo queues, and are very easy to use. os.pipe returns the two file descriptors that act just as you would expect.

The best way to write to pipes is to use a thread. Normally, pipe writing will block until the reader takes data, and this can be a pain to work around, usually requiring the use of "select". I don't know if python has a good/usable implementation of select.

If you use a thread, then both open file handling and feeding the pipe are natural, using a "with ...:" construction. The same thing can support locks by using a "with" inside a "with".

[kiwidude]

@Kovid - the Find Duplicates plugin for binary comparison does two passes.

The first is to add candidates to a map by getting the os stat size and modified datetime:

Spoiler:

    def _find_candidate_by_file_size(self, book_id, candidates_map):
        formats = self.db.formats(book_id, index_is_id=True, verify_formats=False)
        count = 0
        for fmt in formats.split(','):
            fmt_path = self.db.format_abspath(book_id, fmt, index_is_id=True)
            if fmt_path:
                try:
                    stats = os.stat(fmt_path)
                    mtime = stats.st_mtime
                    size = stats.st_size
                    candidates_map[size].add((book_id, fmt, fmt_path, mtime))
                    count += 1
                except:
                    traceback.print_exc()
        return count

The second pass is on the reduced subset (where size and modified datetime match) to compute a hash for each of those books:

Spoiler:

    def _find_candidate_by_hash(self, book_id, fmt, fmt_path, mtime, size, candidates_map, hash_map, result_hash_map):
        # Work out whether we need to calculate a hash for this file from
        # book plugin data from a previous run
        book_data = hash_map.get(book_id, {}).get(fmt, {})
        if book_data.get('mtime', None) == mtime:
            sha = book_data.get('sha', None)
            size = book_data.get('size', None)
            if sha and size:
                candidates_map[(sha, size)].add(book_id)
                self._add_to_hash_map(result_hash_map, book_id, fmt, book_data)
                return
        try:
            with open(fmt_path, 'rb') as f:
                content = f.read()
            sha = hashlib.sha256()
            sha.update(content)
            hash = (sha.hexdigest(), size)
            candidates_map[hash].add(book_id)
            # Store our plugin book data for future repeat scanning
            book_data['mtime'] = mtime
            book_data['sha'] = sha.hexdigest()
            book_data['size'] = size
            self._add_to_hash_map(result_hash_map, book_id, fmt, book_data)
        except:
            traceback.print_exc()

[kovidgoyal]

Hmm, would the file descriptors be inherited in child processes? If so, that is the perfect solution.

Looking at: http://docs.python.org/library/select.html select doesn't work on pipes on windows.