search

flathub / org.desmume.DeSmuME

https://flathub.org/apps/details/org.desmume.DeSmuME
0 stars 7 forks source link

Updates and fixes #3

Closed j8r closed 2 years ago

j8r commented 2 years ago

Various updates and fixes.

~I think it is fine to add filesystem=host:ro, it allows the emulator to work out of the box, and we can say it is fair to trust the emulator by default. It is still possible to restrict this permission afterwards.~

flathubbot commented 2 years ago

Started test build 65950

flathubbot commented 2 years ago

Build 65950 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/63765/org.desmume.DeSmuME.flatpakref
flathubbot commented 2 years ago

Started test build 65951

flathubbot commented 2 years ago

Build 65951 failed

flathubbot commented 2 years ago

Started test build 65954

flathubbot commented 2 years ago

Build 65954 failed

linkmauve commented 2 years ago

Hi,

-minline-all-stringops only exists on x86, which is why your build fails on AArch64 (note that ArchLinux only supports amd64, unlike Flathub which also supports AArch64). You may want to make it depend on that platform, or alternatively (if it is actually useful, btw do you have benchmarks to share?) submit it upstream.

For --filesystem=host:ro though, I made sure to use GtkFileChooserNative everywhere user-facing, did you encounter a place where the old GtkFileChooser was used instead? I tested this flatpak quite extensively back when I submitted it and it behaved pretty much identical to running DeSmuME without a sandbox, wrt user interaction.

Good catch for --device=all and --share=ipc! I use neither controllers nor X11 so I didn’t encounter those issues. Do you have a benchmark of the X11 performance increase btw?

flathubbot commented 2 years ago

Started test build 65957

flathubbot commented 2 years ago

Build 65957 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/63772/org.desmume.DeSmuME.flatpakref
j8r commented 2 years ago

--filesystem=host:ro is necessary to run from the command line. It may be considered as an edge case though.

j8r commented 2 years ago

@linkmauve Ready for review!

flathubbot commented 2 years ago

Started test build 66889

flathubbot commented 2 years ago

Build 66889 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/64716/org.desmume.DeSmuME.flatpakref
flathubbot commented 2 years ago

Started test build 66896

flathubbot commented 2 years ago

Build 66896 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/64723/org.desmume.DeSmuME.flatpakref
j8r commented 2 years ago

@linkmauve ready for review!

linkmauve commented 2 years ago

Hi, have you tested the effects of the -minline-all-stringsops and the two additional meson options? I’m a bit worried about cargo culting such options if they don’t actually improve anything, but if you have tested them and they provide a measurable improvement then LGTM!

Locally I already see -O2 being passed to gcc without this PR, for instance.

You may also want to update to latest master, I just merged a few PRs upstream which help with quality of life.

flathubbot commented 2 years ago

Started test build 67660

flathubbot commented 2 years ago

Build 67660 successful To test this build, install it from the testing repository:

flatpak install --user https://dl.flathub.org/build-repo/65487/org.desmume.DeSmuME.flatpakref
j8r commented 2 years ago

Hard to measure the difference with the optimizations and without, the change was made in https://aur.archlinux.org/cgit/aur.git/commit/?h=desmume-git&id=a5dadb5c1b50e014c5b77f3f7d9a9336d236307b.

From the article https://newbedev.com/why-is-this-code-6-5x-slower-with-optimizations-enabled -minline-all-stringops

By default GCC inlines string operations only when the destination is known to be aligned to least a 4-byte boundary. This enables more inlining and increases code size, but may improve performance of code that depends on fast memcpy, strlen, and memset for short lengths.

linkmauve commented 2 years ago

I don’t trust this AUR commit to have done sufficient research, especially since it includes exactly no comment or info in the commit message about why it added this flag.

I’ll merge it anyway, since that’s already a very useful improvement, we can benchmark later and revert this particular commit if it ends up slower than without the option. Thanks for your contribution!