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

Incorporated faster line count #11

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@llogiq
Contributor

llogiq commented Sep 23, 2016

Thanks for the hint; here's the faster linecount for your ripgrep utility.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 23, 2016

Contributor

See newlinebench for more information.

Contributor

llogiq commented Sep 23, 2016

See newlinebench for more information.

@Byron

This comment has been minimized.

Show comment
Hide comment
@Byron

Byron Sep 25, 2016

I am so looking forward to see this PR land, just because line counts are enabled by default and are something people do expect to see anyway. Looking at the benchmarks on @BurntSushi 's blog, being faster at this will be absolutely noticeable !

Byron commented Sep 25, 2016

I am so looking forward to see this PR land, just because line counts are enabled by default and are something people do expect to see anyway. Looking at the benchmarks on @BurntSushi 's blog, being faster at this will be absolutely noticeable !

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Sep 25, 2016

Owner

It will likely only be noticeable when searching very large files.

But yes, I believe @llogiq is working on putting this in a crate since there have been other developments. I would also like to understand the algorithm and audit it for safety before merging, so it might be a bit. Hang tight.

Owner

BurntSushi commented Sep 25, 2016

It will likely only be noticeable when searching very large files.

But yes, I believe @llogiq is working on putting this in a crate since there have been other developments. I would also like to understand the algorithm and audit it for safety before merging, so it might be a bit. Hang tight.

@Byron

This comment has been minimized.

Show comment
Hide comment
@Byron

Byron Sep 25, 2016

Sounds very reasonable ! I will happily use the fastest and best 'out-of-the' best experience then, to soon be able to tout it just became a little bit faster :).

Byron commented Sep 25, 2016

Sounds very reasonable ! I will happily use the fastest and best 'out-of-the' best experience then, to soon be able to tout it just became a little bit faster :).

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 25, 2016

Contributor

Yeah, I want to strip down the use of unsafe, put that in its own crate, document the bit twiddling and the rationale better and then I'll update.

Of course you can pull this now (as it'll already be faster), and I'll push a new PR when it's ready.

Contributor

llogiq commented Sep 25, 2016

Yeah, I want to strip down the use of unsafe, put that in its own crate, document the bit twiddling and the rationale better and then I'll update.

Of course you can pull this now (as it'll already be faster), and I'll push a new PR when it's ready.

Show outdated Hide outdated Cargo.toml
@@ -1,33 +1,26 @@
[package]

This comment has been minimized.

@BurntSushi

BurntSushi Sep 27, 2016

Owner

I don't understand why you rewrote my Cargo.toml? Could you please not do that? (There are several stylistic choices that I don't like.)

@BurntSushi

BurntSushi Sep 27, 2016

Owner

I don't understand why you rewrote my Cargo.toml? Could you please not do that? (There are several stylistic choices that I don't like.)

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Sep 27, 2016

Owner

@llogiq Thanks! I have a nit about changes to Cargo.toml.

Other than that, I need to audit bytecount before I can merge this. I'm not sure when I'll get to that. Maybe this weekend? It would help if there was an explanation of how it works. cc @Veedrac

Owner

BurntSushi commented Sep 27, 2016

@llogiq Thanks! I have a nit about changes to Cargo.toml.

Other than that, I need to audit bytecount before I can merge this. I'm not sure when I'll get to that. Maybe this weekend? It would help if there was an explanation of how it works. cc @Veedrac

@Veedrac

This comment has been minimized.

Show comment
Hide comment
@Veedrac

Veedrac Sep 27, 2016

@BurntSushi I've got a new version that should be easier to audit.

The basic idea is that the function vector_not turns any null bytes in a usize into 1s and any others into 0s. I go along four usizes at a time, adding these into a counts array. Every 255 quadruplets, I flush the bytewise counts into the total count to prevent any byte from overflowing.

Veedrac commented Sep 27, 2016

@BurntSushi I've got a new version that should be easier to audit.

The basic idea is that the function vector_not turns any null bytes in a usize into 1s and any others into 0s. I go along four usizes at a time, adding these into a counts array. Every 255 quadruplets, I flush the bytewise counts into the total count to prevent any byte from overflowing.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 27, 2016

Contributor

I have a somewhat thorough explanation on my blog.

Contributor

llogiq commented Sep 27, 2016

I have a somewhat thorough explanation on my blog.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Oct 9, 2016

Contributor

Any updates on this?

Contributor

llogiq commented Oct 9, 2016

Any updates on this?

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Oct 9, 2016

Owner

No.

On Oct 9, 2016 5:25 PM, "llogiq" notifications@github.com wrote:

Any updates on this?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb34gzTxlpTX36PrUZNFnmCevI73D8aks5qyVvkgaJpZM4KEkOo
.

Owner

BurntSushi commented Oct 9, 2016

No.

On Oct 9, 2016 5:25 PM, "llogiq" notifications@github.com wrote:

Any updates on this?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAb34gzTxlpTX36PrUZNFnmCevI73D8aks5qyVvkgaJpZM4KEkOo
.

@Veedrac

This comment has been minimized.

Show comment
Hide comment
@Veedrac

Veedrac Oct 10, 2016

FWIW, a newer version has SIMD support, which is a lot faster.

Veedrac commented Oct 10, 2016

FWIW, a newer version has SIMD support, which is a lot faster.

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Nov 6, 2016

Owner

I merged this in 4368913 with a squash and a regression test for #128. Sorry it took so long @llogiq but thank you!

Owner

BurntSushi commented Nov 6, 2016

I merged this in 4368913 with a squash and a regression test for #128. Sorry it took so long @llogiq but thank you!

@BurntSushi BurntSushi closed this Nov 6, 2016

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Nov 6, 2016

Owner

This did wind up with quite a nice speed up! Comparing old master (rg-master) with rg using avx-accel.

First, a base line with no line counting:

[andrew@Cheetah subtitles] time rg-master 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m1.578s
user    0m1.213s
sys     0m0.363s
[andrew@Cheetah subtitles] time rg 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m1.533s
user    0m1.140s
sys     0m0.390s

And now with line counting:

[andrew@Cheetah subtitles] time rg-master -n 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m4.260s
user    0m3.867s
sys     0m0.397s
[andrew@Cheetah subtitles] time rg -n 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m2.131s
user    0m1.810s
sys     0m0.323s

Twice as fast!

Owner

BurntSushi commented Nov 6, 2016

This did wind up with quite a nice speed up! Comparing old master (rg-master) with rg using avx-accel.

First, a base line with no line counting:

[andrew@Cheetah subtitles] time rg-master 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m1.578s
user    0m1.213s
sys     0m0.363s
[andrew@Cheetah subtitles] time rg 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m1.533s
user    0m1.140s
sys     0m0.390s

And now with line counting:

[andrew@Cheetah subtitles] time rg-master -n 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m4.260s
user    0m3.867s
sys     0m0.397s
[andrew@Cheetah subtitles] time rg -n 'Sherlock Holmes' OpenSubtitles2016.raw.en | wc -l
5107

real    0m2.131s
user    0m1.810s
sys     0m0.323s

Twice as fast!

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Nov 6, 2016

Contributor

Cool!

Contributor

llogiq commented Nov 6, 2016

Cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment