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
[email protected]
http://lists.pwmt.org/mailman/listinfo/zathura
