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

Fudge openmp from homebrew's GCC 7.2 #2

Merged
merged 1 commit into from
Feb 24, 2018
Merged

Fudge openmp from homebrew's GCC 7.2 #2

merged 1 commit into from
Feb 24, 2018

Conversation

kornelski
Copy link
Collaborator

This should make it noticeably faster. However, it's a bit hacky since it needs to link with GCC's library, which brew puts wherever it wants. Maybe it could be improved with injection of brew --prefix somewhere in the config.

@sindresorhus
Copy link
Owner

I don't know a way to use Bash commands in that config, but you should use /usr/local/opt/gcc/lib/gcc/7 instead, which doesn't depend on the version at least.

@kornelski kornelski force-pushed the openmp branch 2 times, most recently from 9a13cc5 to 8bbadee Compare February 22, 2018 15:41
@sindresorhus
Copy link
Owner

I'm getting an error when building:

Undefined symbols for architecture x86_64:
  "_res_9_init", referenced from:
      std::sys::unix::net::res_init_if_glibc_before_2_26::h9ed5e7c128746e75 in libgifski.a(std-ab5ab2cb870d2bce.std15-a873b44fb36798aec8ef4901102ce1fc.rs.rcgu.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

GCC 7.3.0
Rust 1.24.0

@sindresorhus
Copy link
Owner

I stumbled upon an issue of yours while Googling this: rust-lang/rust#46797

@kornelski
Copy link
Collaborator Author

Yup. Link with libresolv. It's in "required libraries" in the gifski xcodeproj subjproject.

@sindresorhus
Copy link
Owner

Ok now it builds, but I'm still getting an error when archiving:

SetOwnerAndGroup sindresorhus:staff /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/InstallationBuildProductsLocation/usr/local/lib/libgifski.a
    cd /Users/sindresorhus/dev/private/gifski-app/gifski-api
    /usr/sbin/chown -RH sindresorhus:staff /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/InstallationBuildProductsLocation/usr/local/lib/libgifski.a

chown: /Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/InstallationBuildProductsLocation/usr/local/lib/libgifski.a: No such file or directory
Command /usr/sbin/chown failed with exit code 1

@kornelski
Copy link
Collaborator Author

that archiving error is bizarre, since the library is not supposed to be installed anywhere and shouldn't be in the archive. I don't know why xcode expects it there.

Does the usual xcode exorcism of cleaning the build folder help?

@kornelski
Copy link
Collaborator Author

Hm, maybe it needs SKIP_INSTALL set

@sindresorhus
Copy link
Owner

Does the usual xcode exorcism of cleaning the build folder help?

No

@sindresorhus
Copy link
Owner

Hm, maybe it needs SKIP_INSTALL set

Still the same error.

@kornelski
Copy link
Collaborator Author

Hacked in 86193d3

@sindresorhus
Copy link
Owner

Now I'm getting this when archiving:

clang: error: no such file or directory: '/Users/sindresorhus/Library/Developer/Xcode/DerivedData/Gifski-fqctospusthmqjhbdupnamoaczvv/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/BuildProductsPath/target/release/libgifski.a'

Are you not seeing these errors?

@kornelski
Copy link
Collaborator Author

Yes, I'm getting the same when archiving. Normal build/run works.

@kornelski
Copy link
Collaborator Author

The issue with archiving is worse. It doesn't even build the library when archiving :(

@kornelski
Copy link
Collaborator Author

No, it's even weirder. It doesn't show it's building the library in the build log, but it does build it.

And then, it symlinks the library with itself!?

ls -l /Users/porneL/Library/Developer/Xcode/DerivedData/Gifski-gyxlsysgukvtlzahwwnbvwsmgzzx/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/BuildProductsPath/target/release/libgifski.a
lrwxr-xr-x 1 porneL staff 207 Feb 22 17:38 /Users/porneL/Library/Developer/Xcode/DerivedData/Gifski-gyxlsysgukvtlzahwwnbvwsmgzzx/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/BuildProductsPath/target/release/libgifski.a -> /Users/porneL/Library/Developer/Xcode/DerivedData/Gifski-gyxlsysgukvtlzahwwnbvwsmgzzx/Build/Intermediates.noindex/ArchiveIntermediates/Gifski/IntermediateBuildFilesPath/UninstalledProducts/macosx/libgifski.a

@kornelski
Copy link
Collaborator Author

I think I've found the cause — Xcode gets confused when build_dir == build_products_dir. I'll tweak the integration to avoid that

sindresorhus added a commit that referenced this pull request Feb 23, 2018
@sindresorhus
Copy link
Owner

@kornelski Can you rebase from master and fix the merge conflict?

@sindresorhus
Copy link
Owner

I tested this PR against release build of master, and it decreases the convert time by ~25%:

openmp release build 14.65 seconds
master release build 19.61 seconds

@sindresorhus
Copy link
Owner

@kornelski You said we could not use GCD as the frames have to be converted in order, I'm then curious how OpenMP is able to achieve this speed improvement. Do you know?

@sindresorhus sindresorhus merged commit 3d61b42 into master Feb 24, 2018
@sindresorhus sindresorhus deleted the openmp branch February 24, 2018 11:20
@kornelski
Copy link
Collaborator Author

kornelski commented Feb 24, 2018

OpenMP affects internals of libimagequant, e.g. remapping of pixels is done in parallel. It's more fine-grained parallelism than what GCD aims for.

Still, because of the inter-frame dependency I'm hitting Amdahl's law hard.

@sindresorhus
Copy link
Owner

Do all frames depend on each other? For example, could we split the a video into two videos, process them in parallel, and then merge them at the end?

@sindresorhus
Copy link
Owner

Could also encode intraframes in parallel.

This is a good read if you haven’t read it already: http://www.drdobbs.com/parallel/optimizing-video-encoding-using-threads/225600370

@kornelski
Copy link
Collaborator Author

In GIFs produced by gifski every frame depends on its previous frame, by design. It's equivalent to a video without keyframes. Encoding of frames themselves is already very parallelized.

Splitting input video into parts (roughly equal to number of cores), encoding parts independently and then stitching together would parallelize GIF creation quite well at cost of adding keyframe-like frames in the GIF that may have lower quality (since they won't be able to correctly reuse background from their previous frame).

I've also considered encoding video in fragments, stitching fragments, and then re-encoding frames on the stitches to reduce quality loss.

However, all of this considerably increases complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants