Bug 223314 - textproc/ripgrep: expose SIMD options
Summary: textproc/ripgrep: expose SIMD options
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jan Beich
URL:
Keywords: patch, patch-ready
Depends on: 223342
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-29 22:40 UTC by Jan Beich
Modified: 2017-11-11 19:02 UTC (History)
3 users (show)

See Also:
petteri.valkonen: maintainer-feedback+


Attachments
v1 (1.19 KB, patch)
2017-10-29 22:40 UTC, Jan Beich
no flags Details | Diff
v1.1 (1.37 KB, patch)
2017-11-01 21:39 UTC, Jan Beich
petteri.valkonen: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Beich freebsd_committer freebsd_triage 2017-10-29 22:40:07 UTC
Created attachment 187574 [details]
v1

ripgrep allows to take advantage of SIMD optimizations. On i386 this implies SSE2 except i686-unknown-freebsd target already forces it, anyway. AVX has to be enabled explicitly probably because simd crate doesn't consult CPUID.

Before (or AVX=off SIMD=off):

  $ rg --version
  ripgrep 0.7.1
  -AVX -SIMD

After (or AVX=off SIMD=on):

  $ rg --version
  ripgrep 0.7.1
  -AVX +SIMD

After (AVX=on SIMD=on):

  $ rg --version
  ripgrep 0.7.1
  +AVX +SIMD

Build logs (AVX=on SIMD=on):

  10.3 amd64 http://sprunge.us/FEiH
  10.3 i386  http://sprunge.us/BUMb
  11.0 amd64 http://sprunge.us/jKAY
  11.0 i386  http://sprunge.us/iYEb
  12.0 amd64 http://sprunge.us/LdNZ

Benchmark:

  # 10.3-RELEASE i386 with ripgrep AVX=off SIMD=off
  $ cd /usr/ports
  $ time -l rg -l XXX >/dev/null
	  0.90 real         1.84 user         3.35 sys
       41808  maximum resident set size
	2595  average shared memory size
	  44  average unshared data size
	 128  average unshared stack size
	9383  page reclaims
	   0  page faults
	   0  swaps
       85597  block input operations
	   0  block output operations
	   0  messages sent
	   0  messages received
	   0  signals received
	1249  voluntary context switches
	1778  involuntary context switches

  # 10.3-RELEASE i386 with ripgrep AVX=on SIMD=on
  $ cd /usr/ports
  $ time -l rg -l XXX >/dev/null
	  0.63 real         1.57 user         3.23 sys
       41528  maximum resident set size
	2580  average shared memory size
	  44  average unshared data size
	 128  average unshared stack size
	9315  page reclaims
	   0  page faults
	   0  swaps
       54207  block input operations
	   0  block output operations
	   0  messages sent
	   0  messages received
	   0  signals received
	1880  voluntary context switches
	1270  involuntary context switches

Note, either AVX=on or SIMD=on requires lang/rust built with PORT_LLVM=off (default).
Note2, SIMD=on builds fine on aarch64 but nothing currently uses NEON.
Comment 1 Jan Beich freebsd_committer freebsd_triage 2017-10-29 22:49:40 UTC
Rust codegen can also be forced to emit SIMD optimizations e.g.,

  # /etc/make.conf
  CPUTYPE ?= native
  RUSTFLAGS += -C target-cpu=${CPUTYPE}
  .export RUSTFLAGS # for www/firefox

but this is outside of scope.
Comment 2 John Hein 2017-10-31 11:46:03 UTC
(In reply to Jan Beich from comment #0)
Why is PORT_LLVM=off required?
Comment 3 Jan Beich freebsd_committer freebsd_triage 2017-10-31 13:05:02 UTC
(In reply to John Hein from comment #2)
cfg_target_feature is broken if rust 1.21+ is built with any other LLVM version than bundled (see bug 223300) but simd crate relies on it.
Comment 4 John Hein 2017-10-31 13:28:13 UTC
(In reply to John Hein from comment #2)
I think maybe I see why rust needs its own bundled llvm version... something like this patch depends on a rust llvm target-features extension crate that is not part of the devel/llvmXY port?  Is this something that could be added to the llvm port, or is it really something rust-specific that makes no sense to add to the llvm port?
Comment 5 John Hein 2017-10-31 13:30:32 UTC
(In reply to Jan Beich from comment #3)
Thanks - understood.  I left comment 4 before seeing this, but is this target-feature extension not something that could/should be added to the llvm port?
Comment 6 Jan Beich freebsd_committer freebsd_triage 2017-10-31 16:06:57 UTC
(In reply to John Hein from comment #5)
That's up to maintainers (see bug 223342). cfg_target_feature isn't stable yet. It's not even clear whether Rust stabilization implies relying on stable APIs. SIMD=on here relies on RUSTC_BOOTSTRAP=1 hack to unlock unstable features. packages-only users, i386 users, aarch64 users and 12.0-CURRENT users currently don't have a usable rust-nightly.
Comment 7 Jan Beich freebsd_committer freebsd_triage 2017-10-31 17:02:07 UTC
(In reply to Jan Beich from comment #0)
> On i386 this implies SSE2 except i686-unknown-freebsd target already forces it, anyway.

Per https://github.com/rust-lang/rust/blob/1.21.0/src/librustc_back/target/i686_unknown_freebsd.rs#L16
Comment 8 Petteri Valkonen 2017-11-01 20:59:09 UTC
(In reply to Jan Beich from comment #0)

I'm afraid I'm not familiar enough with Rust's internals to offer any meaningful feedback on this patch. It seems you and John have a good discussion going, though.

However, what I can comment on is that I'd like to see the RUSTC_BOOTSTRAP=1 hack be better documented in the Makefile itself (i.e. why is it needed?), and not just be mentioned in passing in the PR.
Comment 9 Jan Beich freebsd_committer freebsd_triage 2017-11-01 21:39:34 UTC
Created attachment 187662 [details]
v1.1

(In reply to petteri.valkonen from comment #8)
> I'm afraid I'm not familiar enough with Rust's internals to offer
> any meaningful feedback on this patch.

Neither me. I don't use Rust outside of firefox or ripgrep.

Are you going to approve or not? The patch here can land before bug 223342.

> It seems you and John have a good discussion going, though.

The ports framework currently doesn't support depending on specific options in dependencies, so individual ports work around by managing defaults or using slaves. For one, databases/sqlite3 has a number of options changing which will break consumers. textproc/ripgrep depends on PORT_LLVM=off in lang/rust which is already default, so this is mostly FYI. Besides, PORT_LLVM maybe dropped if it proves to be hard to maintain such as more Rust features start to depend on APIs specific to bundled LLVM.

> I'd like to see the RUSTC_BOOTSTRAP=1 hack be better documented in
> the Makefile itself (i.e. why is it needed?), and not just be
> mentioned in passing in the PR.

"# cheat Nightly requirement" wasn't good enough? OK, I've made it more verbose.
Comment 10 Petteri Valkonen 2017-11-02 21:15:42 UTC
Comment on attachment 187662 [details]
v1.1

LGTM.
Comment 11 commit-hook freebsd_committer freebsd_triage 2017-11-02 21:48:55 UTC
A commit references this bug:

Author: jbeich
Date: Thu Nov  2 21:48:21 UTC 2017
New revision: 453382
URL: https://svnweb.freebsd.org/changeset/ports/453382

Log:
  textproc/ripgrep: expose SIMD options

  PR:		223314
  Approved by:	Petteri Valkonen (maintainer)

Changes:
  head/textproc/ripgrep/Makefile
Comment 12 commit-hook freebsd_committer freebsd_triage 2017-11-11 19:02:37 UTC
A commit references this bug:

Author: tobik
Date: Sat Nov 11 19:01:56 UTC 2017
New revision: 453990
URL: https://svnweb.freebsd.org/changeset/ports/453990

Log:
  textproc/ripgrep: Restore BASH, FISH, ZSH options

  They were broken by r453382 which introduced new AVX, SIMD options
  which overwrote the old ones.

  PR:		223478, 223314
  Submitted by:	petteri.valkonen@iki.fi (maintainer)

Changes:
  head/textproc/ripgrep/Makefile