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) {
> +     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 %ld\n", 
> 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.

> +
> +   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.
#0  zathura_page_get_widget (zathura=0x2aaab8881bd0, page=0x2aaab63e5830) at 
utils.c:325
325       return zathura->pages[page_number];
(gdb) bt
#0  zathura_page_get_widget (zathura=0x2aaab8881bd0, page=0x2aaab63e5830) at 
utils.c:325
#1  0x00002aaaaaace5b6 in zathura_page_cache_lru_invalidate 
(zathura=0x2aaaaabc1ba0) at zathura.c:1224
#2  zathura_page_cache_add (zathura=zathura@entry=0x2aaaaabc1ba0, 
page=page@entry=0x2aaab8881bd0) at zathura.c:1251
#3  0x00002aaaaaab9dc9 in cb_view_vadjustment_value_changed 
(UNUSED_adjustment=<optimized out>, data=0x2aaaaabc1ba0) at callbacks.c:99
#4  0x00002aaaac0876e0 in g_closure_invoke (closure=0x2aaaaabcefb0, 
return_value=0x0, n_param_values=1, param_values=0x7ffffdd276d0, 
invocation_hint=0x7ffffdd27670) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:777
#5  0x00002aaaac098750 in signal_emit_unlocked_R 
(node=node@entry=0x2aaaaabe6240, detail=detail@entry=0, 
instance=instance@entry=0x2aaaaabcd2f0, 
emission_return=emission_return@entry=0x0, 
instance_and_params=instance_and_params@entry=0x7ffffdd276d0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3551
#6  0x00002aaaac0a06bc in g_signal_emit_valist (instance=0x2aaaaabcd2f0, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd27918) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3300
#7  0x00002aaaac0a0852 in g_signal_emit 
(instance=instance@entry=0x2aaaaabcd2f0, signal_id=<optimized out>, 
detail=detail@entry=0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#8  0x00002aaaab19961a in IA__gtk_adjustment_value_changed 
(adjustment=0x2aaaaabcd2f0) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkadjustment.c:668
#9  0x00002aaaac0876e0 in g_closure_invoke (closure=0x2aaaaab9b4c0, 
return_value=0x0, n_param_values=1, param_values=0x7ffffdd27bd0, 
invocation_hint=0x7ffffdd27b70) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:777
#10 0x00002aaaac098750 in signal_emit_unlocked_R 
(node=node@entry=0x2aaaaabe6160, detail=detail@entry=0, 
instance=instance@entry=0x2aaaaabcd2f0, 
emission_return=emission_return@entry=0x0, 
instance_and_params=instance_and_params@entry=0x7ffffdd27bd0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3551
#11 0x00002aaaac0a06bc in g_signal_emit_valist (instance=0x2aaaaabcd2f0, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd27e18) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3300
#12 0x00002aaaac0a0852 in g_signal_emit 
(instance=instance@entry=0x2aaaaabcd2f0, signal_id=<optimized out>, 
detail=detail@entry=0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#13 0x00002aaaab19954b in IA__gtk_adjustment_changed 
(adjustment=adjustment@entry=0x2aaaaabcd2f0) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkadjustment.c:660
#14 0x00002aaaab36876b in gtk_viewport_size_allocate (widget=0x2aaaaabcde90, 
allocation=0x7ffffdd28350) at /tmp/buildd/gtk+2.0-2.24.10/gtk/gtkviewport.c:841
#15 0x00002aaaac08a86e in g_cclosure_marshal_VOID__BOXEDv 
(closure=0x2aaaaabc8d60, return_value=<optimized out>, instance=0x2aaaaabcde90, 
args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, 
param_types=0x2aaaaabc8db0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gmarshal.c:1160
#16 0x00002aaaac0878db in _g_closure_invoke_va (closure=0x2aaaaabc8d60, 
return_value=0x0, instance=0x2aaaaabcde90, args=0x7ffffdd28278, n_params=1, 
param_types=0x2aaaaabc8db0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:840
#17 0x00002aaaac0a0006 in g_signal_emit_valist (instance=0x2aaaaabcde90, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd28278) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3211
#18 0x00002aaaac0a0852 in g_signal_emit 
(instance=instance@entry=0x2aaaaabcde90, signal_id=<optimized out>, 
detail=detail@entry=0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#19 0x00002aaaab370ec8 in IA__gtk_widget_size_allocate (widget=0x2aaaaabcde90, 
allocation=allocation@entry=0x7ffffdd283e0) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwidget.c:4089
#20 0x00002aaaab2b76fa in gtk_scrolled_window_size_allocate 
(widget=0x2aaaaabe5cc0, allocation=0x7ffffdd28830) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkscrolledwindow.c:1436
#21 0x00002aaaac08a86e in g_cclosure_marshal_VOID__BOXEDv 
(closure=0x2aaaaabc8d60, return_value=<optimized out>, instance=0x2aaaaabe5cc0, 
args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, 
param_types=0x2aaaaabc8db0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gmarshal.c:1160
#22 0x00002aaaac0878db in _g_closure_invoke_va (closure=0x2aaaaabc8d60, 
return_value=0x0, instance=0x2aaaaabe5cc0, args=0x7ffffdd28758, n_params=1, 
param_types=0x2aaaaabc8db0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:840
#23 0x00002aaaac0a0006 in g_signal_emit_valist (instance=0x2aaaaabe5cc0, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd28758) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3211
#24 0x00002aaaac0a0852 in g_signal_emit 
(instance=instance@entry=0x2aaaaabe5cc0, signal_id=<optimized out>, 
detail=detail@entry=0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#25 0x00002aaaab370ec8 in IA__gtk_widget_size_allocate (widget=0x2aaaaabe5cc0, 
allocation=allocation@entry=0x7ffffdd288c0) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwidget.c:4089
#26 0x00002aaaab1a406a in gtk_box_size_allocate (widget=0x2aaaaabcfed0, 
allocation=0x7ffffdd28d10) at /tmp/buildd/gtk+2.0-2.24.10/gtk/gtkbox.c:500
#27 0x00002aaaac08a86e in g_cclosure_marshal_VOID__BOXEDv 
(closure=0x2aaaaabc8d60, return_value=<optimized out>, instance=0x2aaaaabcfed0, 
args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, 
param_types=0x2aaaaabc8db0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gmarshal.c:1160
#28 0x00002aaaac0878db in _g_closure_invoke_va (closure=0x2aaaaabc8d60, 
return_value=0x0, instance=0x2aaaaabcfed0, args=0x7ffffdd28c38, n_params=1, 
param_types=0x2aaaaabc8db0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:840
#29 0x00002aaaac0a0006 in g_signal_emit_valist (instance=0x2aaaaabcfed0, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd28c38) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3211
#30 0x00002aaaac0a0852 in g_signal_emit 
(instance=instance@entry=0x2aaaaabcfed0, signal_id=<optimized out>, 
detail=detail@entry=0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#31 0x00002aaaab370ec8 in IA__gtk_widget_size_allocate (widget=0x2aaaaabcfed0, 
allocation=allocation@entry=0x7ffffdd28d60) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwidget.c:4089
#32 0x00002aaaab37b42a in gtk_window_size_allocate (widget=0x2aaaaabcf930, 
allocation=0x7ffffdd29290) at /tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwindow.c:4994
#33 0x00002aaaac0876e0 in g_closure_invoke (closure=0x2aaaaabc8d60, 
return_value=0x0, n_param_values=2, param_values=0x7ffffdd28f60, 
invocation_hint=0x7ffffdd28f00) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:777
#34 0x00002aaaac098073 in signal_emit_unlocked_R 
(node=node@entry=0x2aaaaabc8dd0, detail=detail@entry=0, 
instance=instance@entry=0x2aaaaabcf930, 
emission_return=emission_return@entry=0x0, 
instance_and_params=instance_and_params@entry=0x7ffffdd28f60) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3481
#35 0x00002aaaac0a06bc in g_signal_emit_valist (instance=0x2aaaaabcf930, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd291b8) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3300
#36 0x00002aaaac0a0852 in g_signal_emit 
(instance=instance@entry=0x2aaaaabcf930, signal_id=<optimized out>, 
detail=detail@entry=0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#37 0x00002aaaab370ec8 in IA__gtk_widget_size_allocate 
(widget=widget@entry=0x2aaaaabcf930, 
allocation=allocation@entry=0x2aaaaabcf970) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwidget.c:4089
#38 0x00002aaaab1d7a95 in IA__gtk_container_resize_children 
(container=container@entry=0x2aaaaabcf930) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkcontainer.c:1478
#39 0x00002aaaab37c0e6 in gtk_window_move_resize (window=0x2aaaaabcf930) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwindow.c:6403
#40 gtk_window_check_resize (container=0x2aaaaabcf930) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkwindow.c:5408
#41 0x00002aaaac0879a7 in _g_closure_invoke_va (closure=0x2aaaaabcfd30, 
return_value=0x0, instance=0x2aaaaabcf930, args=0x7ffffdd296a8, n_params=0, 
param_types=0x0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gclosure.c:840
#42 0x00002aaaac0a0006 in g_signal_emit_valist (instance=0x2aaaaabcf930, 
signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7ffffdd296a8) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3211
#43 0x00002aaaac0a0852 in g_signal_emit (instance=<optimized out>, 
signal_id=<optimized out>, detail=<optimized out>) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./gobject/gsignal.c:3356
#44 0x00002aaaab1d7a20 in gtk_container_idle_sizer (data=<optimized out>) at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkcontainer.c:1357
#45 0x00002aaaab775327 in gdk_threads_dispatch (data=0x2aaab6402e60) at 
/tmp/buildd/gtk+2.0-2.24.10/gdk/gdk.c:512
#46 0x00002aaaac310355 in g_main_dispatch (context=0x2aaaaabc0890) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./glib/gmain.c:2539
#47 g_main_context_dispatch (context=context@entry=0x2aaaaabc0890) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./glib/gmain.c:3075
#48 0x00002aaaac310688 in g_main_context_iterate (context=0x2aaaaabc0890, 
block=block@entry=1, dispatch=dispatch@entry=1, self=<error reading variable: 
Unhandled dwarf expression opcode 0xfa>) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./glib/gmain.c:3146
#49 0x00002aaaac310a82 in g_main_loop_run (loop=0x2aaaaadaf9e0) at 
/tmp/buildd/glib2.0-2.33.12+really2.32.4/./glib/gmain.c:3340
#50 0x00002aaaab252797 in IA__gtk_main () at 
/tmp/buildd/gtk+2.0-2.24.10/gtk/gtkmain.c:1256
#51 0x00002aaaaaab8e1a in main (argc=1, argv=0x7ffffdd29cc8) at main.c:151

> +
> +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?

Cheers
-- 
Sebastian Ramacher
_______________________________________________
zathura mailing list
zathura@lists.pwmt.org
http://lists.pwmt.org/mailman/listinfo/zathura

Reply via email to