543255 - Unused cairo_private functions

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Ehren Metcalfe]

Attachment: remove functions

There are a number of unused functions in the cairo library. Removing them results in an admittedly modest decrease of 1500 bytes from libxul. 

I was told that it may be more appropriate to #ifdef 0 unused code in any C libraries but I've posted a patch with the functions removed. I suppose it may make more sense to file this sort of stuff upstream but I'm looking for the community's input since this is quite likely not the only dead C code in the tree.

Functions removed:

cairo-cache.c:
_cairo_hash_bytes

cairo-path-fixed.c:
_cairo_path_fixed_hash
_cairo_path_fixed_size
_cairo_path_fixed_equal

cairo-pattern.c:
_cairo_solid_pattern_hash
_cairo_gradient_color_stops_hash
_cairo_linear_pattern_hash
_cairo_radial_pattern_hash
_cairo_surface_pattern_hash
_cairo_pattern_hash
_cairo_gradient_pattern_color_stops_size
_cairo_pattern_size
_cairo_solid_pattern_equal
_cairo_gradient_color_stops_equal
_cairo_linear_pattern_equal
_cairo_radial_pattern_equal
_cairo_surface_pattern_equal
_cairo_pattern_equal

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 424440 [details] [diff] [review]
remove functions

I'll let jeff make the final call here, but I don't think we should do this unless this patch gets applied to upstream cairo as well -- otherwise tracking merges will be painful for not much good reason.

Comment 2

[Bas Schouten (:bas.schouten)]

Hrm, if this is unused code how come it's taking so much space in libxul? In theory shouldn't the linker notice that the code is not exposed externally and not called internally, and rip all of it out of the library? With the right linker settings in theory dead code elimination should take care of this.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 424440 [details] [diff] [review]
remove functions

Indeed, merging this would make tracking cairo upstream harder than it needs to be. Further, removing unused functions is the linker's job.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #1)
> (From update of attachment 424440 [details] [diff] [review])
> I'll let jeff make the final call here, but I don't think we should do this
> unless this patch gets applied to upstream cairo as well -- otherwise tracking
> merges will be painful for not much good reason.

Indeed, we use -dead_strip on OS X which does this. I assume the win32 linker does the right thing too. On Linux, (which Ehren was testing on?) we could/should use '-ffunction-sections -Wl,-gc-sections'

Comment 5

[Ehren Metcalfe]

> we could/should use '-ffunction-sections -Wl,-gc-sections'

Actually bug 537857 has already been filed for this.

Is '-ffunction-sections -Wl,-gc-sections' smart enough to make these removals though?

Just to test I ran a few comparison builds on rev 49cc24d8bd6b and compared the size of libxul with the following results (I'm on Linux x86_64 btw):

using -ffunction-sections -Wl,-gc-sections:

with the removal patch:
29079688 bytes

without:
29082888 bytes

difference: 3200 bytes

----

without using -ffunction-sections -Wl,-gc-sections:

with the patch:
31050385 bytes

without:
31051889 bytes

difference: 1504 bytes

----

I also tried '-fdata-sections -ffunction-sections -Wl,-gc-sections':

with the path:

29028476 bytes

without:

29033244 bytes

difference: 4768 bytes

Odd that the disparity widens with these flags...

I completely understand that this would be a tracking nightmare just to save 1 or 2 k though

Comment 6

[Jeff Muizelaar [:jrmuizel]]

If you run 'nm' on the resulting binary you should be able to find out whether these symbols included or not.

Comment 7

[Ehren Metcalfe]

Here's what I found with nm:

0000000000d89580 t _cairo_pattern_equal
0000000000d894d0 t _cairo_gradient_color_stops_equal
0000000000d89770 t _cairo_pattern_hash
0000000000d89050 t _cairo_pattern_size
0000000000d89700 t _cairo_gradient_color_stops_hash
0000000000d85040 t _cairo_path_fixed_equal
0000000000d84730 t _cairo_path_fixed_size
0000000000d85bf0 t _cairo_path_fixed_hash
0000000000dbe2a0 t _cairo_hash_bytes

These were found in the libxul compiled with '-fdata-sections -ffunction-sections -Wl,-gc-sections'

Comment 8

[Marco Castelluccio [:marco]]

Mark as WONTFIX or send the patch to Cairo?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Please file a new bug about the unused functions not being stripped.