roualdes / bridgestan

BridgeStan provides efficient in-memory access through Python, Julia, and R to the methods of a Stan model.
https://roualdes.github.io/bridgestan
BSD 3-Clause "New" or "Revised" License
87 stars 12 forks source link

Set up visibility controls for public symbols in the library #185

Closed WardBrian closed 8 months ago

WardBrian commented 8 months ago

This PR makes the necessary changes to set the default symbol visibility to "hidden". It's worth reading more about this on the GCC Wiki.

The summary:

WardBrian commented 8 months ago

What is the use case of BRIDGESTAN_EXPORT? It doesn't seem like we use it, or maybe I just missed that.

It gets defined on line 23 of the makefile. The purpose is such that when we're building a shared library on Windows, the functions are marked as dllexport, but if somebody else writes #include <bridgestan.h>, they get a dllimport-marked function instead.

Should this have some doc about it?

It could, but I'm not sure what to write or where it would be best suited. Is the idea we'd be documenting it for end users or for developers/ourselves?

size measurements

Binary size (using model ode_sundials):

Optization setting Size in bytes, before Size in bytes, after
O3 1,108,800 1,016,888
O2 1,065,152 969,376
Os 1,041,056 860,928

Number of symbols (nm -C -D ode_sundials_model.so | wc -l). These are both at O3, it mattered much less for this in terms of total numbers:

Before After
1559 956

Note: I picked this model as sort of a worst-case, since the sundials symbols are exported. For something like bernoulli, the "after" is ~500 symbols.

speed

This is harder since we don't have a ton of precise benchmarks. At a basic level, things like loading the library will get faster (less to load into memory, and less symbols to scan over to find our functions), but testing to see if this genuinely provides an increased opportunity for optimizations is difficult

roualdes commented 8 months ago

Re BRIDGESTAN_EXPORT. Thanks. That makes sense.

This seems great. Thanks again for finding this and thanks for incorporating it into BridgeStan.