Re: [zathura] [Zathura PATCH] Replace the periodic page reclaiming code with a LRU caching algorithm.
On Mon, Mar 25, 2013 at 07:44:10PM +0100, Sebastian Ramacher wrote:
> First of all, thanks for the awesome patch and implementing this
> superior method of page caching.
You,re more than welcome.
> Here are some comments.
>
> On 2013-03-25 13:11:21, Marwan Tanager wrote:
> > diff --git a/page.c b/page.c
> > index d31af46..cb84313 100644
> > --- a/page.c
> > +++ b/page.c
> > @@ -11,15 +11,6 @@
> > #include "internal.h"
> > #include "types.h"
> >
> > -struct zathura_page_s {
> > - double height; /**< Page height */
> > - double width; /**< Page width */
> > - unsigned int index; /**< Page number */
> > - void* data; /**< Custom data */
> > - bool visible; /**< Page is visible */
> > - zathura_document_t* document; /**< Document */
> > -};
> > -
> > zathura_page_t*
> > zathura_page_new(zathura_document_t* document, unsigned int index,
> > zathura_error_t* error)
> > {
> > diff --git a/page.h b/page.h
> > index dbac902..80d509d 100644
> > --- a/page.h
> > +++ b/page.h
> > @@ -8,6 +8,15 @@
> >
> > #include "types.h"
> >
> > +struct zathura_page_s {
> > + double height; /**< Page height */
> > + double width; /**< Page width */
> > + unsigned int index; /**< Page number */
> > + void* data; /**< Custom data */
> > + bool visible; /**< Page is visible */
> > + zathura_document_t* document; /**< Document */
> > +};
> > +
>
> I didn't see any change the needs access to one of zathura_page_t's
> members. Is this really necessary?
Actually, it's not necessary for the caching code itself, but I needed to do
that in order to be able to access the zathura_page_s.index member for
debugging purposes. Without it, the compiler will issue "dereferencing pointer
to incomplete type" errors with the girara_debug lines. But it turned out that
I've just overlooked the zathura_page_get_index function, which would've
relieved us from moving things around.
> > /**
> > * Get the page object
> > *
> > diff --git a/zathura.c b/zathura.c
> > index 8e8c4da..e650485 100644
> > --- a/zathura.c
> > +++ b/zathura.c
> > @@ -53,7 +53,6 @@ typedef struct position_set_delayed_s {
> > } position_set_delayed_t;
> >
> > static gboolean document_info_open(gpointer data);
> > -static gboolean purge_pages(gpointer data);
> >
> > /* function implementation */
> > zathura_t*
> > @@ -264,11 +263,6 @@ zathura_init(zathura_t* zathura)
> >zathura->bookmarks.bookmarks =
> > girara_sorted_list_new2((girara_compare_function_t)
> > zathura_bookmarks_compare,
> > (girara_free_function_t)
> > zathura_bookmark_free);
> >
> > - /* add even to purge old pages */
> > - int interval = 30;
> > - girara_setting_get(zathura->ui.session, "page-store-interval",
> > &interval);
> > - g_timeout_add_seconds(interval, purge_pages, zathura);
> > -
> >/* jumplist */
> >
> >zathura->jumplist.max_size = 20;
> > @@ -279,6 +273,13 @@ zathura_init(zathura_t* zathura)
> >zathura->jumplist.cur = NULL;
> >zathura_jumplist_append_jump(zathura);
> >zathura->jumplist.cur = girara_list_iterator(zathura->jumplist.list);
> > +
> > + /* page cache */
> > +
> > + zathura->page_cache.size = ZATHURA_PAGE_CACHE_DEFAULT_SIZE;
> > + girara_setting_get(zathura->ui.session, "page-cache-size",
> > &zathura->page_cache.size);
> > + zathura->page_cache.cache = g_malloc(zathura->page_cache.size *
> > sizeof(zathura_page_t*));
>
> Would it make a big difference if we only store indices here? This might
> help with the segfault that I see (details below).
I don't think it will make a difference. (Again!) I've just overlooked the fact
that I could've used the zathura_document_get_page function from the caching
code.
I'll see about that.
> >return true;
> >
> > error_free:
> > @@ -353,6 +354,8 @@ zathura_free(zathura_t* zathura)
> > girara_list_iterator_free(zathura->jumplist.cur);
> >}
> >
> > + g_free(zathura->page_cache.cache);
> > +
> >g_free(zathura);
> > }
> >
> > @@ -1066,30 +1069,6 @@ page_widget_set_mode(zathura_t* zathura, unsigned
> > int pages_per_row, unsigned in
> >gtk_widget_show_all(zathura->ui.page_widget);
> > }
> >
> > -static
> > -gboolean purge_pages(gpointer data)
> > -{
> > - zathura_t* zathura = data;
> > - if (zathura == NULL || zathura->document == NULL) {
> > -return TRUE;
> > - }
> > -
> > - int threshold = 0;
> > - girara_setting_get(zathura->ui.session, "page-store-threshold",
> > &threshold);
> > - if (threshold <= 0) {
> > -return TRUE;
> > - }
> > -
> > - girara_debug("purging pages ...");
> > - unsigned int number_of_pages =
> > zathura_document_get_number_of_pages(zathura->document);
> > - for (unsigned int page_id = 0; page_id < number_of_pages; page_id++) {
> > -zathura_page_t* page = zathura_document_get_page(zathura->document,
> > page_id);
> > -GtkWidget* page_widget = zathura_page_get_widget(zathura, page);
> > -zathura_page_widget_purge_unused(ZATHURA_PAGE(page_widget), t
Re: [zathura] [Zathura PATCH] Replace the periodic page reclaiming code with a LRU caching algorithm.
First of all, thanks for the awesome patch and implementing this
superior method of page caching.
Here are some comments.
On 2013-03-25 13:11:21, Marwan Tanager wrote:
> diff --git a/page.c b/page.c
> index d31af46..cb84313 100644
> --- a/page.c
> +++ b/page.c
> @@ -11,15 +11,6 @@
> #include "internal.h"
> #include "types.h"
>
> -struct zathura_page_s {
> - double height; /**< Page height */
> - double width; /**< Page width */
> - unsigned int index; /**< Page number */
> - void* data; /**< Custom data */
> - bool visible; /**< Page is visible */
> - zathura_document_t* document; /**< Document */
> -};
> -
> zathura_page_t*
> zathura_page_new(zathura_document_t* document, unsigned int index,
> zathura_error_t* error)
> {
> diff --git a/page.h b/page.h
> index dbac902..80d509d 100644
> --- a/page.h
> +++ b/page.h
> @@ -8,6 +8,15 @@
>
> #include "types.h"
>
> +struct zathura_page_s {
> + double height; /**< Page height */
> + double width; /**< Page width */
> + unsigned int index; /**< Page number */
> + void* data; /**< Custom data */
> + bool visible; /**< Page is visible */
> + zathura_document_t* document; /**< Document */
> +};
> +
I didn't see any change the needs access to one of zathura_page_t's
members. Is this really necessary?
> /**
> * Get the page object
> *
> diff --git a/zathura.c b/zathura.c
> index 8e8c4da..e650485 100644
> --- a/zathura.c
> +++ b/zathura.c
> @@ -53,7 +53,6 @@ typedef struct position_set_delayed_s {
> } position_set_delayed_t;
>
> static gboolean document_info_open(gpointer data);
> -static gboolean purge_pages(gpointer data);
>
> /* function implementation */
> zathura_t*
> @@ -264,11 +263,6 @@ zathura_init(zathura_t* zathura)
>zathura->bookmarks.bookmarks =
> girara_sorted_list_new2((girara_compare_function_t) zathura_bookmarks_compare,
> (girara_free_function_t)
> zathura_bookmark_free);
>
> - /* add even to purge old pages */
> - int interval = 30;
> - girara_setting_get(zathura->ui.session, "page-store-interval", &interval);
> - g_timeout_add_seconds(interval, purge_pages, zathura);
> -
>/* jumplist */
>
>zathura->jumplist.max_size = 20;
> @@ -279,6 +273,13 @@ zathura_init(zathura_t* zathura)
>zathura->jumplist.cur = NULL;
>zathura_jumplist_append_jump(zathura);
>zathura->jumplist.cur = girara_list_iterator(zathura->jumplist.list);
> +
> + /* page cache */
> +
> + zathura->page_cache.size = ZATHURA_PAGE_CACHE_DEFAULT_SIZE;
> + girara_setting_get(zathura->ui.session, "page-cache-size",
> &zathura->page_cache.size);
> + zathura->page_cache.cache = g_malloc(zathura->page_cache.size *
> sizeof(zathura_page_t*));
Would it make a big difference if we only store indices here? This might
help with the segfault that I see (details below).
>return true;
>
> error_free:
> @@ -353,6 +354,8 @@ zathura_free(zathura_t* zathura)
> girara_list_iterator_free(zathura->jumplist.cur);
>}
>
> + g_free(zathura->page_cache.cache);
> +
>g_free(zathura);
> }
>
> @@ -1066,30 +1069,6 @@ page_widget_set_mode(zathura_t* zathura, unsigned int
> pages_per_row, unsigned in
>gtk_widget_show_all(zathura->ui.page_widget);
> }
>
> -static
> -gboolean purge_pages(gpointer data)
> -{
> - zathura_t* zathura = data;
> - if (zathura == NULL || zathura->document == NULL) {
> -return TRUE;
> - }
> -
> - int threshold = 0;
> - girara_setting_get(zathura->ui.session, "page-store-threshold",
> &threshold);
> - if (threshold <= 0) {
> -return TRUE;
> - }
> -
> - girara_debug("purging pages ...");
> - unsigned int number_of_pages =
> zathura_document_get_number_of_pages(zathura->document);
> - for (unsigned int page_id = 0; page_id < number_of_pages; page_id++) {
> -zathura_page_t* page = zathura_document_get_page(zathura->document,
> page_id);
> -GtkWidget* page_widget = zathura_page_get_widget(zathura, page);
> -zathura_page_widget_purge_unused(ZATHURA_PAGE(page_widget), threshold);
> - }
> - return TRUE;
> -}
> -
> static gboolean
> position_set_delayed_impl(gpointer data)
> {
> @@ -1215,3 +1194,72 @@ zathura_jumplist_save(zathura_t* zathura)
> cur->y = gtk_adjustment_get_value(view_vadjustment) /
> zathura_document_get_scale(zathura->document);;
>}
> }
> +
> +static bool
> +zathura_page_cache_is_cached(zathura_t* zathura, zathura_page_t* page)
> +{
> + g_return_val_if_fail(zathura != NULL, false);
> +
> + unsigned int i;
> +
> + for (i = 0; i < zathura->page_cache.size; ++i) {
> +if (page == zathura->page_cache.cache[i]) {
> + return true;
> +}
> + }
> +
> + return false;
> +}
> +
> +static ssize_t
> +zathura_page_cache_lru_invalidate(zathura_t* zathura)
> +{
> + g_return_val_if_fail(zathura != NULL, -1);
> +
> + ssize_t lru_index = 0;
> + guint64 view_time = 0;
> + guint64 lru_view_time = G_MAXUINT64;
> +
> + for (unsigned int i = 0; i < zathura->page_cache.size; ++i) {
> + GtkWid
