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), 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) {
> > +     GtkWidget* page_widget = zathura_page_get_widget(zathura, 
> > zathura->page_cache.cache[i]);
> > +     g_object_get(G_OBJECT(page_widget), "last-view", &view_time, NULL);
> > +
> > +     if (view_time < lru_view_time) {
> > +       lru_view_time = view_time;
> > +       lru_index = i;
> > +     }
> > +   }
> > +
> > +   
> > zathura_page_widget_update_surface(ZATHURA_PAGE(zathura_page_get_widget(zathura,
> >  zathura->page_cache.cache[lru_index])), NULL);
> > +   girara_debug("Invalidated page %d at cache index %ldn", 
> > zathura->page_cache.cache[lru_index]->index + 1, lru_index);
> 
> girara_debug adds a n add the end. There's no need to pass an extra n
> here.

Yeah. To your surprise, you will find that Mutt is the culprit here. When I 
sent the patch, I copied the name of Ignas Anikevičius in the annotation. The 
Latin letter made git-send-email change the encoding of the message from the 
usual 7-bit us-ascii to 8-bit utf-8. If you read the message from the list 
archive using a web browser, you will see that the seemingly erranous 'n' that 
appears with Mutt in girara_debug is actually '\n'. I don't know how on earth 
could Mutt edit the message body itself, rather than just ignoring things it 
can't understand?!

But, I've just discovered that girara_debug actually appends a newline for us, 
so that is not necessary anyway.

> > +
> > +   return lru_index;
> > +}
> 
> There is a case that triggers a segfault in
> zathura_page_cache_lru_invalidate. If I have a document open and open another
> one I get the following (sorry for the really long lines):

> Program terminated with signal 11, Segmentation fault.

Yeah, I can see that. I think the problem has to do with dereferencing invalid 
pointers in the cache from the remnant of the old document, after opening the 
new one.

This will certainly get fixed.

> > +void
> > +zathura_page_cache_add(zathura_t* zathura, zathura_page_t* page)
> > +{
> > +  g_return_if_fail(zathura != NULL);
> > +
> > +  static unsigned int num_cached_pages = 0;
> 
> Could you move this to the struct for the cache in zathura_t?

Sure, this would be better.

Thanks, Sebastian for the review, and stay tuned for a second version.


                
        Marwan
_______________________________________________
zathura mailing list
zathura@lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura

Reply via email to