Memchr in std #30381

fhahn · 2015-12-14T23:47:39Z

This PR adds memchrand memrchr based on @BurntSushi 's rust-memchr crate to libstd (as discussed in #30151).

I've update some places in libstd to use memchr/memrchr, but I am not sure if there are other places where it could be used as well.

ref #30076

bluss · 2015-12-14T23:56:41Z

src/libstd/memchr.rs

@@ -0,0 +1,339 @@
+/* Initial implementation taken from Andrew Gallant's rust-memchr


Needs a real copyright header, see other files.

If you put in burntsushi's name, I want to be credited too -- the pure rust memchr impls are mine. bluss is fine.

I've added the rust copyright header, as well as rust-memchrs license header.

@bluss I'd be glad to put your name in as well, what should I put in? It's not in the rust-memchr copyright notice.

bluss · 2015-12-15T00:02:34Z

I think this is exciting, should be a good perf boost to stdout when writing binary data.

bluss · 2015-12-15T00:15:16Z

You can write in (C) bluss or another way to indicate authorship. We should resolve the license question and skip the separate license notice I think.

bluss · 2015-12-15T00:19:05Z

@nicokoch Do you agree with dual-licensing your contributions to rust-memchr under MIT and Apache 2.0 so that it can be included in rust?

@BurntSushi Do you agree with dual-licensing your contributions to rust-memchr under MIT and Apache 2.0 so that it can be included in rust?

I myself agree with dual licensing my contributions to rust-memchr 😄.

Those are all three authors according to the git history.

nicokoch · 2015-12-15T00:30:32Z

Exciting, my first mini contribution to rust!

No problem on my end, go ahead and include it.

bluss · 2015-12-15T00:35:35Z

☑️ great, then the little licensing question should be almost cleared, I have no doubt @BurntSushi will be on board as soon as he sees it.

BurntSushi · 2015-12-15T00:40:31Z

Yes, i agree. Also, to be clear, the memchr crate is far more the work of @bluss than myself! Thank you @bluss! (And thank you @fhahn for the PR!)

bluss · 2015-12-15T00:48:56Z

☑️ relicensing done, well done team ❤️

fhahn · 2015-12-15T10:49:17Z

I've update the license stuff, memchr.rs now includes the Rust license header on top and a comment below which indicates the file initially comes from rust-memchr with an copyright attribution for the three contributers.

bluss · 2015-12-15T16:54:14Z

src/libstd/memchr.rs

+// Copyright 2015 Andrew Gallant, bluss and Nicolas Koch
+
+
+extern crate libc;


This is not needed (libc is already imported in lib.rs)

alexcrichton · 2015-12-16T06:58:57Z

src/libstd/memchr.rs

+    // libc memchr
+    #[cfg(any(not(target_os = "windows"),
+              not(any(target_pointer_width = "32",
+                      target_pointer_width = "64"))))]


I think it's probably safe to say that if we're porting to a platform which doesn't have 32 or 64-bit pointers this function will be the least of our worries. For now could these clauses in the cfg annotations just be removed?

(ping on removing these cfgs)

fhahn · 2015-12-16T23:55:45Z

I've update the commit. I also added a PR to libc that adds memrchr. But this means we must update the libc submodule in the repo, right?

In the libc PR I also add memrchr bindings for Free, Net and OpenBSD, because they are provided there as well. They do not seem to ship handwritten assembler version (as GUN libc does), which means they probably won't beat the Rust implementation by a large margin.

alexcrichton · 2015-12-17T18:31:00Z

Also yeah updating the submodule is fine, should be fine to do as soon as the libc PR lands.

fhahn · 2015-12-17T23:41:04Z

I've updated the submodule and the new libc binding of memrchr is used.

alexcrichton · 2015-12-18T01:41:05Z

@bors: r+ d21888d650f9a72e862ec469d755674938a6b7a9

Thanks @fhahn!

Manishearth · 2015-12-18T10:26:56Z

http://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt/builds/2428/steps/compile/logs/stdio

fhahn · 2015-12-18T12:39:20Z

@Manishearth I've moved the libc import to the functions that use it

- Successful merges: #30272, #30286, #30365, #30381, #30384, #30398, #30406, #30408, #30420, #30431, #30447, #30452 - Failed merges:

bluss · 2015-12-18T15:42:14Z

From travis log, the doc examples need updating (or ignoring)


---- memchr::memchr_0 stdout ----
    <anon>:4:9: 4:15 error: unresolved import `memchr::memchr`. Maybe a missing `extern crate memchr`? [E0432]
<anon>:4     use memchr::memchr;
                 ^~~~~~
error: aborting due to previous error
thread 'memchr::memchr_0' panicked at 'Box<Any>', src/libsyntax/diagnostic.rs:240

---- memchr::memrchr_0 stdout ----
    <anon>:4:9: 4:15 error: unresolved import `memchr::memrchr`. Maybe a missing `extern crate memchr`? [E0432]
<anon>:4     use memchr::memrchr;
                 ^~~~~~
error: aborting due to previous error
thread 'memchr::memrchr_0' panicked at 'Box<Any>', src/libsyntax/diagnostic.rs:240


failures:
    memchr::memchr_0
    memchr::memrchr_0

fhahn · 2015-12-18T20:50:30Z

I've added ignore to the doc tests

alexcrichton · 2015-12-18T20:53:30Z

@bors: r+ a206e55

bluss · 2015-12-18T21:31:31Z

Thanks for working on this!

bors · 2015-12-19T00:57:26Z

⌛ Testing commit a206e55 with merge 8ad12c3...

@BurntSushi

This PR adds `memchr`and `memrchr` based on @BurntSushi 's rust-memchr crate to libstd (as discussed in #30151). I've update some places in libstd to use memchr/memrchr, but I am not sure if there are other places where it could be used as well. ref #30076

bors · 2015-12-19T02:45:14Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

SimonSapin · 2015-12-22T12:49:20Z

These new functions are private to std, right?

If they are to become public, I’d prefer to have a more descriptive name, similar to how copy_nonoverlapping is not named memcpy.

bluss · 2015-12-22T12:55:33Z

Yep they are an implementation detail.

The relnotes-worthy thing is the performance improvements. We'll probably expand the use of these functions soon.

bluss · 2015-12-22T12:56:46Z

relnotes related: Improve throughput for LineWriter (and thus default stdout).

Use the fallback impl for memrchr on non-linux The memrchr code was never used(!). This brings the memrchr improvements to non-linux platforms (LineWriter / buffered stdout benefits). Previous PR #30381

bluss reviewed Dec 14, 2015
View reviewed changes

bluss added the relnotes label Dec 15, 2015

fhahn force-pushed the memchr-in-std branch from 80ac56c to 570756b Compare December 15, 2015 00:06

fhahn force-pushed the memchr-in-std branch 3 times, most recently from e49137c to 40900af Compare December 15, 2015 10:47

bluss reviewed Dec 15, 2015
View reviewed changes

fhahn force-pushed the memchr-in-std branch from 40900af to 2ab30ff Compare December 15, 2015 22:29

alexcrichton reviewed Dec 16, 2015
View reviewed changes

fhahn force-pushed the memchr-in-std branch from 2ab30ff to 7aff040 Compare December 16, 2015 23:48

fhahn force-pushed the memchr-in-std branch from de130a3 to d21888d Compare December 17, 2015 23:41

alexcrichton self-assigned this Dec 18, 2015

Manishearth mentioned this pull request Dec 18, 2015

Rollup of 12 pull requests #30457

Merged

fhahn added 3 commits December 18, 2015 13:32

Add memchr implemenation based on rust-memchr to libstd

ca52c56

Use memchr in libstd where possible, closes rust-lang#30076

de3e843

Update liblibc to current master

aa1f8fd

fhahn force-pushed the memchr-in-std branch from d21888d to f73a01b Compare December 18, 2015 12:38

bors added a commit that referenced this pull request Dec 18, 2015

Auto merge of #30457 - Manishearth:rollup, r=Manishearth

3391630

- Successful merges: #30272, #30286, #30365, #30381, #30384, #30398, #30406, #30408, #30420, #30431, #30447, #30452 - Failed merges:

Use memrchr bindings provided by libc

a206e55

fhahn force-pushed the memchr-in-std branch from f73a01b to a206e55 Compare December 18, 2015 20:49

bors merged commit a206e55 into rust-lang:master Dec 19, 2015

This was referenced Dec 19, 2015

Use the getentropy(2) syscall on OpenBSD #30430

Merged

std: Remove rust_builtin C support library #30175

Merged

Initial emscripten work #30453

Closed

fhahn deleted the memchr-in-std branch December 22, 2015 23:31

bluss mentioned this pull request Jan 20, 2016

Use the fallback impl for memrchr on non-linux #31057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memchr in std #30381

Memchr in std #30381

fhahn commented Dec 14, 2015

bluss Dec 14, 2015

fhahn Dec 15, 2015

bluss commented Dec 15, 2015

bluss commented Dec 15, 2015

bluss commented Dec 15, 2015

nicokoch commented Dec 15, 2015

bluss commented Dec 15, 2015

BurntSushi commented Dec 15, 2015

bluss commented Dec 15, 2015

fhahn commented Dec 15, 2015

bluss Dec 15, 2015

alexcrichton Dec 16, 2015

alexcrichton Dec 17, 2015

fhahn commented Dec 16, 2015

alexcrichton commented Dec 17, 2015

fhahn commented Dec 17, 2015

alexcrichton commented Dec 18, 2015

Manishearth commented Dec 18, 2015

fhahn commented Dec 18, 2015

bluss commented Dec 18, 2015

fhahn commented Dec 18, 2015

alexcrichton commented Dec 18, 2015

bluss commented Dec 18, 2015

bors commented Dec 19, 2015

bors commented Dec 19, 2015

SimonSapin commented Dec 22, 2015

bluss commented Dec 22, 2015

bluss commented Dec 22, 2015

		@@ -0,0 +1,339 @@
		/* Initial implementation taken from Andrew Gallant's rust-memchr

		// Copyright 2015 Andrew Gallant, bluss and Nicolas Koch


		extern crate libc;

Memchr in std #30381

Memchr in std #30381

Conversation

fhahn commented Dec 14, 2015

bluss Dec 14, 2015

Choose a reason for hiding this comment

fhahn Dec 15, 2015

Choose a reason for hiding this comment

bluss commented Dec 15, 2015

bluss commented Dec 15, 2015

bluss commented Dec 15, 2015

nicokoch commented Dec 15, 2015

bluss commented Dec 15, 2015

BurntSushi commented Dec 15, 2015

bluss commented Dec 15, 2015

fhahn commented Dec 15, 2015

bluss Dec 15, 2015

Choose a reason for hiding this comment

alexcrichton Dec 16, 2015

Choose a reason for hiding this comment

alexcrichton Dec 17, 2015

Choose a reason for hiding this comment

fhahn commented Dec 16, 2015

alexcrichton commented Dec 17, 2015

fhahn commented Dec 17, 2015

alexcrichton commented Dec 18, 2015

Manishearth commented Dec 18, 2015

fhahn commented Dec 18, 2015

bluss commented Dec 18, 2015

fhahn commented Dec 18, 2015

alexcrichton commented Dec 18, 2015

bluss commented Dec 18, 2015

bors commented Dec 19, 2015

bors commented Dec 19, 2015

SimonSapin commented Dec 22, 2015

bluss commented Dec 22, 2015

bluss commented Dec 22, 2015