After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 332992 - Lazily create folded and sort-keys for RBRefStrings
Lazily create folded and sort-keys for RBRefStrings
Status: RESOLVED FIXED
Product: rhythmbox
Classification: Other
Component: general
HEAD
Other Linux
: Normal normal
: ---
Assigned To: RhythmBox Maintainers
RhythmBox Maintainers
: 334495 340854 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-03-01 13:37 UTC by James "Doc" Livingston
Modified: 2006-05-06 21:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (7.24 KB, patch)
2006-03-01 13:40 UTC, James "Doc" Livingston
none Details | Review
use g_atomic_* instead of mutexes (7.50 KB, patch)
2006-03-12 08:44 UTC, James "Doc" Livingston
committed Details | Review
rb-type-punned-refstring.patch (1.89 KB, patch)
2006-03-13 19:27 UTC, Bastien Nocera
none Details | Review
type-punning patch (1.70 KB, patch)
2006-03-14 10:42 UTC, James "Doc" Livingston
committed Details | Review

Description James "Doc" Livingston 2006-03-01 13:37:06 UTC
Currently the folded version of, and sort-key for, RBRefStrings are created in rb_refstring_new. Many of these many not ever get used, or many not be needed immediately. Lazy creation of them would save memory (and potentiall time).
Comment 1 James "Doc" Livingston 2006-03-01 13:40:04 UTC
Created attachment 60397 [details] [review]
patch

The speed differences caused by locking, uninlining and not doing folding/sort-keys are all negligable.

The size of RB's heap immediately after startup is 3/4Mb smaller for me with this patch applied. Some of that is allocated later, when other folded/sort keys get used, but not all of it is.
Comment 2 James "Doc" Livingston 2006-03-12 08:44:55 UTC
Created attachment 61118 [details] [review]
use g_atomic_* instead of mutexes

Changed to use g_atomic_* instead of mutexes.
Comment 3 Bastien Nocera 2006-03-12 09:48:06 UTC
Don't forget to update the requires in the configure.in
Comment 4 James "Doc" Livingston 2006-03-12 10:02:46 UTC
According to the glib api docs g_atomic_pointer_get and g_atomic_pointer_compare_and_exchange have been present since 2.4, and only g_atomic_*_set were added in 2.10 - so configure.in shouldn't need updating.
Comment 5 Bastien Nocera 2006-03-12 11:40:16 UTC
My mistake.
Comment 6 James "Doc" Livingston 2006-03-13 10:58:33 UTC
I've gone over the code again, and this appears to be okay. So I've committed it to cvs.
Comment 7 Bastien Nocera 2006-03-13 19:26:34 UTC
I'm getting:

rb-refstring.c: In function 'rb_refstring_get_folded':
rb-refstring.c:143: warning: dereferencing type-punned pointer will break strict-aliasing rules
rb-refstring.c: In function 'rb_refstring_get_sort_key':
rb-refstring.c:168: warning: dereferencing type-punned pointer will break strict-aliasing rules
Comment 8 Bastien Nocera 2006-03-13 19:27:32 UTC
Created attachment 61200 [details] [review]
rb-type-punned-refstring.patch
Comment 9 James "Doc" Livingston 2006-03-14 10:42:28 UTC
Created attachment 61218 [details] [review]
type-punning patch

That patch breaks the thread-safety of the code, as val->folded and val->sortkey must only be accessed with the g_atomic_pointer_* functions.

I think this should fix the type-punning warnings.
Comment 10 Alex Lancaster 2006-03-14 12:21:22 UTC
*** Bug 334495 has been marked as a duplicate of this bug. ***
Comment 11 James "Doc" Livingston 2006-03-18 12:09:14 UTC
Committed to cvs.
Comment 12 Ryan P Skadberg 2006-03-18 16:35:18 UTC
Still having the compile issues.

 gcc -DHAVE_CONFIG_H -I. -I. -I.. -DGNOMELOCALEDIR=\"/usr/share/locale\" -DG_LOG_DOMAIN=\"RhythmDB\" -I.. -I../lib -I../metadata -DORBIT2=1 -pthread -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libgnomeui-2.0 -I/usr/include/libgnome-2.0 -I/usr/include/libgnomecanvas-2.0 -I/usr/include/libart-2.0 -I/usr/include/gconf/2 -I/usr/include/libbonoboui-2.0 -I/usr/include/gnome-vfs-2.0 -I/usr/lib/gnome-vfs-2.0/include -I/usr/include/gnome-keyring-1 -I/usr/include/orbit-2.0 -I/usr/include/libbonobo-2.0 -I/usr/include/bonobo-activation-2.0 -I/usr/include/freetype2 -I/usr/include/libxml2 -I/usr/include/libglade-2.0 -I/usr/include/gnome-vfs-module-2.0 -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2 -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i686 -mtune=generic -fasynchronous-unwind-tables -Wcomment -Wformat -Wnonnull -Wimplicit-int -Wimplicit -Wmain -Wmissing-braces -Wparentheses -Wsequence-point -Wreturn-type -Wswitch -Wtrigraphs -Wunused-function -Wunused-label -Wunused-value -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wall -Werror -std=gnu89 -MT rb-refstring.lo -MD -MP -MF .deps/rb-refstring.Tpo -c rb-refstring.c  -fPIC -DPIC -o .libs/rb-refstring.o
cc1: warnings being treated as errors
rb-refstring.c: In function 'rb_refstring_get_folded':
rb-refstring.c:139: warning: dereferencing type-punned pointer will break strict-aliasing rules
rb-refstring.c: In function 'rb_refstring_get_sort_key':
rb-refstring.c:166: warning: dereferencing type-punned pointer will break strict-aliasing rules
make[3]: *** [rb-refstring.lo] Error 1
make[3]: Leaving directory `/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603181122/rhythmdb'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603181122/rhythmdb'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/usr/src/redhat/BUILD/rhythmbox-0.9.3.99.cvs200603181122'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.81507 (%build)


Comment 13 Ryan P Skadberg 2006-03-21 00:55:53 UTC
Still getting this with today's CVS.
Comment 14 Ryan P Skadberg 2006-03-22 03:34:58 UTC
Patch from Johnathan committed.
Comment 15 Jonathan Matthew 2006-05-06 21:53:47 UTC
*** Bug 340854 has been marked as a duplicate of this bug. ***