Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Co-installable double-builds #1995

Merged
merged 18 commits into from Jun 23, 2023
Merged

Co-installable double-builds #1995

merged 18 commits into from Jun 23, 2023

Conversation

umlaeute
Copy link
Contributor

@umlaeute umlaeute commented Jun 16, 2023

this PR deals with the idea that both a single-precision Pd and a double-precision Pd can be installed at the same time (with the same package/installer/...).

The PR mostly fixes bugs with the buildsystem, that prevent the creation of co-installable binaries.

from a "new feature" perspective, I think the sole new feature is that

  • the GUI now has a preference to select which variant is to be launched
  • and some accompanying code to actually launch that variant (either "pd" or "pd64")
  • this "new feature" has been added as the last commit (4036292), so if it is unfit for inclusion in Pd-0.54-0, it can be easily left out)

after a lot of discussing and experimenting (see below), it also turned out that it's best that double-precision and single-precision do not share the same filename space (that is: they should use different file-extensions as a way to never accidentally try to load a double-precision external into a single-precision runtime; this was mostly problematic on Windows, where a single-precision library would force a single-precision runtime by means of loading the single-precision pd.dll).
the simple solution for that was to not use the good ole' library extensions (e.g. .pd_linux) for double-precision externals at all.

the remainder is really

for practical reasons (to resolve a merge conflict), this includes the (current) develop branch...


the main idea was laid out by @danomatika on the mailinglist and expanded by @Lucarda later on:

  • ship both pd.exe (for the single-precision build) and pd64.exe (for the double-precision build)
  • have a single Pd-GUI
    • but have some option in the Pd-GUI preferences to tell it which variant of Pd to launch (if Pd is started via the GUI)

this PR doesn't automatically build double-precision binaries (this still has to be done explicitly by whoever creates the packages)

@umlaeute umlaeute requested a review from danomatika June 16, 2023 20:03
@umlaeute umlaeute changed the base branch from master to develop June 16, 2023 20:04
@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 16, 2023

the basic workflow I imagine is like this:

  1. build a single-precision Pd.app

    ./configure
    make app

    store away the created app with something like tar cf single.tar pd-*/

  2. build a double-precision Pd.app

    ./configure --with-floatsize=64 --program-transform-name="s/pd$/pd64/"
    make app

    store away the created app with something like tar cf double.tar pd-*/

  3. overlay the two apps

    rm -rf pd-*/
    tar xf double.tar
    tar xf single.tar
  4. create an distributable (Windows installer, macOS image) from the overlay

typically i would build the single- and double-precision version either on two independent machines (for the CI), or in two separate build directories (locally):

mkdir build-single
cd build-single
../configure
make app
tar cf app.tar ?d-*/
cd ..

mkdir build-double
cd build-double
../configure --with-floatsize=64 --program-transform-name="s/pd$/pd64/"
make app
tar cf app.tar ?d-*/
cd ..

for d in build-double build-single; do
  tar xf "${d}/app.tar"
done

ping @Lucarda for testing.

the Windows installer-builder (build-nsi.sh) still expects a bin/pd.exe file so it will fail if you run it on a build that only contains the pd64.exe (the suggested double-precision build) and no pd.exe.
i don't think this is a real problem.

@Lucarda
Copy link
Contributor

Lucarda commented Jun 16, 2023

ping @Lucarda for testing.

fa

fan

fanta

fantastic!

I builded with:

./configure
make app
tar cf single.tar pd-*/
make clean
./configure --with-floatsize=64 --program-transform-name="s/pd$/pd64/"
make app
tar cf double.tar pd-*/
make clean
rm -rf pd-*/
tar xf single.tar
tar xf double.tar

It works like a charm. :)

I had also tested the GUI startup pref.

This is wonderful!

@Lucarda

This comment was marked as resolved.

@porres
Copy link
Contributor

porres commented Jun 17, 2023

tested and it seems great

now other than testing the 3.audio.examples/B15.tabread4~-onset example I don't know how to check in a patch wether we have a different precision. I thought number boxes would show larger numbers or more digits, but they still have the same look (and shouldn't it be different tough?). Any other hints?

@umlaeute

This comment was marked as resolved.

@umlaeute umlaeute linked an issue Jun 17, 2023 that may be closed by this pull request
@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 17, 2023

@porres they look pretty much the same (i never picked katja's formatting changes for double-precision).
other hints to check if you are running a double-precision Pd:

  • the deken-format specifier (as shown in either the deken-preferences; or at startup if you raise the verbosity to "4 all") should say 64 rather than 32
  • deken will no longer find most libraries (as they are now of a foreign architecture)
  • trying to load a (legacy, single-precision) library should give you an error.

@Lucarda
Copy link
Contributor

Lucarda commented Jun 17, 2023

trying to load a (legacy, single-precision) library should give you an error.

@umlaeute I was about to report on this. I thought I would get an error but I got a crash while loading a single-precision dll

verbose(4): tried F:/git-portable-2/pd-artnetlib/artnetlib.windows-amd64-64.dll and failed
verbose(4): tried F:/git-portable-2/pd-artnetlib/artnetlib.windows-amd64-0.dll and failed
verbose(4): tried F:/git-portable-2/pd-artnetlib/artnetlib.m_amd64 and failed
verbose(4): tried F:/git-portable-2/pd-artnetlib/artnetlib.dll and succeeded

crash.

I'm on pd64.

note that in this path there's no file artnetlib.m_amd64

@umlaeute umlaeute marked this pull request as draft June 18, 2023 07:26
@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 18, 2023

yikes. this probably means that the entire idea of having a pd64.exe besides the pd.exe is doomed to fail.
i guess what happens is:

  • pd64.exe starts and loads pd64.dll?
    • pd64.dll provides the double-precision runtime
  • you load the artnetlib library
    • the artnetlib.dll that is eventually found has a dependency on pd.dll
    • the dynamic linker thus loads the pd.dll, which provides a colliding set of symbols
    • pd.dll provides the single-precision runtime
  • we now have to colliding runtimes and crashes are imminent

possibly we need to ressort to a different filesystem-scheme, like

  • bin/pd.exe
  • bin/pd.dll
  • bin-double/pd.exe
  • bin-double/pd.dll

@danomatika
Copy link
Contributor

danomatika commented Jun 18, 2023 via email

@umlaeute
Copy link
Contributor Author

@danomatika yes of course

@Lucarda could you check, whether removing the pd.dll from your bin/ folder fixes the crash?

@Lucarda
Copy link
Contributor

Lucarda commented Jun 18, 2023

@Lucarda could you check, whether removing the pd.dll from your bin/ folder fixes the crash?

Yes. renaming or moving the file fixes the crash.

@Lucarda
Copy link
Contributor

Lucarda commented Jun 18, 2023

One simple thing that we can do is rename the bin folder right after compiling the double app.

rm -rf pd-*/
./configure
make app
tar cf single.tar pd-*/
make clean
./configure --with-floatsize=64 --program-transform-name="s/pd$/pd64/"
make app
cd pd-* && mv bin bin64 && cd ../
tar xf single.tar

then some magic has to be done so that the GUI handles starting the right stuff. The Windows installed app always start wish8*.exe from the bin folder.

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 18, 2023

if we rename the directory, there's no need to rename the binary (with --program-transform-name="s/pd$/pd64/")

that is, it should be as simple as

./configure --with-floatsize=64
make app bindir='${exec_prefix}/bin64'

@Lucarda
Copy link
Contributor

Lucarda commented Jun 18, 2023

I'm going to test build that right now.

@Spacechild1
Copy link
Contributor

Spacechild1 commented Jun 18, 2023

possibly we need to ressort to a different filesystem-scheme, like

bin/pd.exe
bin/pd.dll
bin-double/pd.exe
bin-double/pd.dll

I would really try to avoid having different naming schemes on different platforms (bin/pd64 on Linux vs bin64/pd.exe on Windows) and I would prefer to keep both pd.exe and pd64.exe in the bin folder.


I am sure this has been discussed already, but why don't we have a dedicated mandatory file extension for double precision externals? (FWIW, SuperCollider does this to distinguish scsynth plugins from supernova plugins.) This would solve our pd.dll problem because pd64.exe wouldn't even try to load double-precision externals and vice versa.

Another big advantage: single-precision and double-precision externals could co-exist and even be shipped as a single Deken package.

What are the downsides?

(Alternatively, we could have a different name of the setup function. Although this could be done with a simple macro, it would require slight modification of all existing external sources...)

Generally, I think it is the responsibility of the host application to only select suitable plugins - and not just load optimistically and let it error out. (The class_new64 mechanism is still a useful additional safety check.)

PS: How do you currently deal with the extra externals when shipping both versions in the same package? Wouldn't the single-precision externals be replaced by the double-precision variant (or vice versa)?

@Lucarda
Copy link
Contributor

Lucarda commented Jun 18, 2023

it turns out we have:

./bin
./bin/libwinpthread-1.dll
./bin/msvcr90.dll
./bin/msvcrt.dll
./bin/pdfontloader.dll
./bin/pthreadVC.dll
./bin/pthreadVC.lib
./bin/tcl85.dll
./bin/tcl85.lib
./bin/tclpip85.dll
./bin/tclsh85.exe
./bin/tclstub85.lib
./bin/tk85.dll
./bin/tk85.lib
./bin/tkstub85.lib
./bin/wish85.exe
./bin64
./bin64/pd-gui
./bin64/pd.com
./bin64/pd.def
./bin64/pd.dll
./bin64/pd.exe
./bin64/pd.lib
./bin64/pdreceive.exe
./bin64/pdsend.exe

if I start ./bin64/pd.com it asks me for libwinpthread-1.dll and if i copy it to bin64, to my surprise, it works.


@Spacechild1 said

but why don't we have a dedicated mandatory file extension for double precision externals?

I agree with this.


I'll be offline for a couple of hours.

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 18, 2023

Another big advantage: single-precision and double-precision externals could co-exist and even be shipped as a single Deken package.

with the current scheme, they can co-exist and even be shipped as a single deken package.
they could even be shipped in a single binary.

Alternatively, we could have a different name of the setup function.

i'm sure i said this before, but: this just doesn't work.
there are externals out there that do not rely on the setup function being called in order to register their classes.

PS: How do you currently deal with the extra externals when shipping both versions in the same package? Wouldn't the single-precision externals be replaced by the double-precision variant (or vice versa)?

they ship with different extensions.

What are the downsides?

it adds additional technical debt to the host application.
the extension handling code is already quite complicated (and I'd like to keep the handling as uniform as possible)

@Spacechild1
Copy link
Contributor

they could even be shipped in a single binary.

Wouldn't this require different setup functions? I think it was actually me who had this idea. My thought was that you could just compile the source twice and then link it together; you would have to use a macro around the setup function which would expand to a different name for the double-precision version.

with the current scheme, they can co-exist and even be shipped as a single deken package.

Sorry, maybe I am stupid or not up to date, but let's say I have a 64-bit Windows external foo and want to ship both versions, what would be the (different) extensions?

i'm sure i said this before, but: this just doesn't work.
there are externals out there that do not rely on the setup function being called in order to register their classes.

To be clear: I am talking about the entry point of the external. Every external binary has a (single) entry point. Or can you point me to an exception?

@Spacechild1
Copy link
Contributor

Spacechild1 commented Jun 18, 2023

it adds additional technical debt to the host application.
the extension handling code is already quite complicated (and I'd like to keep the handling as uniform as possible)

One nice thing about a new mandatory extension for double precision externals would be that we could just ignore all the legacy extensions and only allow a single possible extension, e.g. .m_amd64.dp for Windows Intel 64-bit double precision. So I think that this could actually simplify the loading process (at least on Pd double).

@umlaeute
Copy link
Contributor Author

One nice thing about a new mandatory extension for double precision externals would be that we could just ignore all the legacy extensions and only allow a single possible extension, e.g. .m_amd64.dp for Windows Intel 64-bit double precision. So I think that this could actually simplify the loading process (at least on Pd double).

the file extension as layed out for now is .windows-amd64-64.dll (let's not rehash this discussion).

what i do not want to add is specific code to check whether this is a double-precision build and skip extensions based on that info.

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 18, 2023

Sorry, maybe I am stupid or not up to date, but let's say I have a 64-bit Windows external foo and want to ship both versions, what would be the (different) extensions?

  • .windows-amd64-64.dll (Windows/x86_64/double)
  • .windows-amd64-32.dll (Windows/x86_64/single)
  • .darwin-arm64-64.so (macOS/AppleSilicon/double)
  • .linux-pdp11-32.so (...)

To be clear: I am talking about the entry point of the external. Every external binary has a (single) entry point. Or can you point me to an exception?

it's easy to use other methods than the entry point to execute code.
e.g. instantiate a static C++-object and call some code in the constructor.

they could even be shipped in a single binary.

Wouldn't this require different setup functions?

no.
the external would just need to call class_new() resp class_new64() depending on whether it was called by a single- or double-precision runtime (speaking of which, i think we forgot a sys_get_floatsize() function...)

@Spacechild1
Copy link
Contributor

Spacechild1 commented Jun 18, 2023

the file extension as layed out for now is .windows-amd64-64.dll (let's not rehash this discussion).

Ah, so we do have dedicated file extensions for single/double precisions.

what i do not want to add is specific code to check whether this is a double-precision build and skip extensions based on that info.

But - why not? In my naive view, everything you'd have to do is use an #ifdef in the file extension list. How would it even affect the rest of the loader logic?

IMO if we already have a dedicated file extensions, it would be dumb to not use it for filtering... Why keep the myriads of extensions if we have the chance to make a clean break?

@Spacechild1
Copy link
Contributor

Spacechild1 commented Jun 18, 2023

it's easy to use other methods than the entry point to execute code.
e.g. instantiate a static C++-object and call some code in the constructor.

No. It is not legal to do this. You cannot even expect Pd to be fully initialized at the time the global construtor runs, e.g. if the plugin is linked statically. You must not call any Pd API function before the setup function.

the external would just need to call class_new() resp class_new64() depending on whether it was called by a single- or double-precision runtime (speaking of which, i think we forgot a sys_get_floatsize() function...)

Ah, right, makes sense.

@danomatika
Copy link
Contributor

danomatika commented Jun 23, 2023 via email

@Lucarda
Copy link
Contributor

Lucarda commented Jun 23, 2023

There is something we should take note:

If we start using pd64.dll (as in the combined app) and people start linking to it in externals, then we might not be able to stop shipping it. May be (in the future, if needed) there's a way to make it a kind of symlink to pd.dll.

I prefer to say this now.

@Lucarda
Copy link
Contributor

Lucarda commented Jun 23, 2023

i could also provide the Windows builds (though this currently uses the embedded TclTk, and I think miller is using something else for the releases).

in a wonderful world we should have

pdprototype.tgz tcl/tk 8.6.10 (32-bit)
pdprototype64.tgz tcl/tk 8.6.10 (64-bit)

@danomatika
Copy link
Contributor

danomatika commented Jun 23, 2023 via email

@millerpuckette
Copy link
Contributor

Maybe I'm missing something but... pd64.dll would never be interchangeable with pd.dll because, although both are 64-bit Windows libs, one uses 64-bit audio samples and the other uses 32-bit ones. We need to keep the 32-bit-sample version alive for compatibility with hardware that lacks fast 64-bit arithmetic (including, I believe, Raspberry Pi).
Meanwhile, "pdprototype" doesn't contain any dependencies on the audio samplesize. (It does contain a 32-bit TCL/TK and a few 32-bit libs (pthreadVC, msvcrt, ...) that I believe are only used for the 32-bit-address-space-back-compatibility build. So it can stand as is for now.
Meanwhile, I don't think separating out a 64-bit-samples version requires 7 new builds, only 3 of them, since it's fine to only offer it for the latest platforms (MacOS + PC zip + PC installer).
Implicit int he above, but I should mention: I think the 32-bit-samples 64-bit-pointer version will be the most widely used for a long time to come, since it offers the best compatibility across CPUs.

@porres
Copy link
Contributor

porres commented Jun 23, 2023

(including, I believe, Raspberry Pi).

only older ones actually, new ones are 64 bit

@millerpuckette
Copy link
Contributor

I'm not sure but I believe that even on RPI 4 the arithmetic units take multiple cycles to do 64-bit operations, so 32-bit arithmetic is way faster than 64.

@Spacechild1
Copy link
Contributor

Personally, I don't really have a strong opinion about "fat" installers VS separate installers. The only thing I deeply care about is a01a264.

If we start using pd64.dll (as in the combined app) and people start linking to it in externals, then we might not be able to stop shipping it

Yes, but I think that pd64.dll is fine. We need to use different names for the single-precision and double-precision binaries either way and so far nobody has a proposed a better name. IIRC, csound uses same naming scheme, so it can't be too unreasonable :)

@millerpuckette
Copy link
Contributor

No that I look at the "runtime-selectable precision switch" code I see that it doesn't do anything if Pd is packaged for only one precision type - so there's no reason it can't go into master. I'll just merge the whole banana now...

@millerpuckette millerpuckette merged commit 77416b9 into master Jun 23, 2023
10 checks passed
@Spacechild1
Copy link
Contributor

Spacechild1 commented Jun 23, 2023

I'm not sure but I believe that even on RPI 4 the arithmetic units take multiple cycles to do 64-bit operations

The Cortex-A72 is a 64-bit ARMv8 chip, so that would surprise me...

In general, all Intel and ARM CPUs can use vector registers/instructions for floating point math. 64-bit Intel actually mandates at least SSE2 support. Even with 32-bit Intel you would typically compile with at least SSE support, so there would be no difference between floats and doubles, at least for scalar code.

AFAICT, floats vs. doubles only really makes a difference for vectorized code. SSE registers can hold 4 floats, but only 2 doubles; AVX registers can hold 8 floats, but only 4 doubles.

@Lucarda
Copy link
Contributor

Lucarda commented Jun 23, 2023

Is rather difficult that now we can have a double pd.dll if we had built via

./configure --with-floatsize=64
make app

we should make the name mangling automatic in the autotools script.

@Lucarda
Copy link
Contributor

Lucarda commented Jun 23, 2023

Could it be symlink / alias or the Windows equivalent?

On MINGW we can do:

ln <TARGET>.dll <LINK_NAME>.dll

@Lucarda
Copy link
Contributor

Lucarda commented Jun 23, 2023

We need to use different names for the single-precision and double-precision binaries either way and so far nobody has a proposed a better name. IIRC, csound uses same naming scheme, so it can't be too unreasonable :)

we don't need different names for the binaries. I think Linux does not have them. For Windows we can have same names but different:

  • install folder
  • desktop icon
  • start menu entry

EDIT: and

  • general app name

@Spacechild1
Copy link
Contributor

I think Linux does not have them.

How would you install both Pd versions simultaneously on Linux? I thought both binaries would go to /usr/bin resp. /usr/local/bin, so you would need different names. I am a bit confused now.

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 23, 2023 via email

@Lucarda
Copy link
Contributor

Lucarda commented Jun 23, 2023

@millerpuckette please don't oppose to "both variants in a single app" on Windows and macOS.

@Spacechild1
Copy link
Contributor

On Linux, like on macOS, there is no "pd.dll" equivalent

I know :) With "both binaries" I meant pd and pd64.

Both variants can be co-installed if they have different names.

Ok, that's what I figured.

While we could use the same name on macOS and Windows - even for "universal apps" -, this is not a practical option on Linux. IMO it would be good to have the same naming scheme (pd = single-precision, pd64 = double-precision) on all platforms, just to avoid any confusion ("Why is Pd called pd64 on Linux but pd on Windows?"). Could we just make pd64 the default for double-precision builds?

I think @Lucarda is right that at least on Windows we really have to decide on a canonical name because externals link back to it. We should do that before starting to actually ship "official" double-precision builds.

@umlaeute
Copy link
Contributor Author

Could we just make pd64 the default for double-precision builds?

Sure.
e.g. the puredata64-core package in Debian, already provides a binary /usr/bin/pd64

@umlaeute umlaeute deleted the feature/floatsize-build branch June 23, 2023 22:31
@umlaeute umlaeute mentioned this pull request Jun 26, 2023
@Spacechild1
Copy link
Contributor

Could we just make pd64 the default for double-precision builds?

Sure.

Is this something that needs further discussion - or can we (you) push this to develop (if @millerpuckette agrees)?

@danomatika
Copy link
Contributor

danomatika commented Jun 27, 2023 via email

@umlaeute
Copy link
Contributor Author

Could we just make pd64 the default for double-precision builds?

Sure.

Is this something that needs further discussion - or can we (you) push this to develop (if @millerpuckette agrees)?

oh, i think i actually misunderstood the "default" part.

from my side:

  • all the infrastructure is in place that allows us to build double-precision binaries with pd64 as the binary name (including pd64.dll)
  • the CI (on https://git.iem.at/pd/pure-data) is setup to build double-precision binaries with pd64
  • ./configure will spit out a warning if you try building a double-precision binary without transforming using pd64

from my side this would be enough.

i (now) understand that with your notion of default, you probably do not like the following caveats:

  • it does require an explicit --program-transform-name=s/pd$/pd64/ when calling configure
  • while there is a warning (that suggests what to do), there is no error if you don't

@Spacechild1
Copy link
Contributor

it does require an explicit --program-transform-name=s/pd$/pd64/ when calling configure

Or the less obscure --program-suffix=64 :)

Anyway, the warning is nice and all, but why not just set pd64 as the default name? Power users can still override it if they want to (for whatever reason). IMO it is a bit strange to have A as the default, but tell the user "A is probably not what you want, you should rather do B"...

@umlaeute
Copy link
Contributor Author

because it's slightly more complicated than it should be (it's hard to tell whether the user asked for transforming the program-name)

in any case, i think i have it working now.
will submit to the develop branch

@millerpuckette
Copy link
Contributor

I think for now at least 32-bit samples should be the default. 64-bitness will change the audio output of existing patches which I think needs careful warning and preparation.

@umlaeute
Copy link
Contributor Author

umlaeute commented Jun 27, 2023

Totally.
The default when building Pd is currently single precision (32bit): this is what we know and what has been used/tested for the last decades.

the last few comments were about the "default" filename of the binaries, if (and only if) somebody is brave enough to compile a double-precision Pd by passing --with-floatsize=64 to configure.

@umlaeute umlaeute moved this from In progress to Done in double precision Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
6 participants