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

Maximum and minimum file size #534

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Maximum and minimum file size #534

wants to merge 6 commits into from

Conversation

pdl
Copy link
Contributor

@pdl pdl commented Mar 6, 2015

Follows on from discussion in #529.

  • The only per-file overhead for users who don't use the new options is a single boolean check to see if the size filter exists.
  • No new stats, we use -s _.
  • I can't see anything in there which would break on Windows and the only failures I saw running the tests (a couple of commits back, because I have that running on a different machine) looked like irrelevant stuff for which there are open issues.

@petdance
Copy link
Collaborator

petdance commented Mar 6, 2015

@hoelzro : Please don't do anything with this until we've talked. I think I want to freeze new features until we get bugs under control, even cool new features like this one.

my $n = $1;
if ($2) {
my $u = lc $2;
$n *= 1024 while $u =~ tr/tgmk/gmk/d;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty clever, but it might be too clever; maybe we should just have a hash of suffixes => number of bytes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And are we going to distinguish between MB and MiB, GB and GiB?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I keep being surprised at how decimal things are nowadays. Looks like things like ls -l defaults to 1K=1000 so I guess yes, we should.

@hoelzro
Copy link
Collaborator

hoelzro commented Mar 6, 2015

@pdl Thanks for the patch! I took a look and left a few comments on the changes. In addition, you may want to try using the timings script in the dev directory to see how your changes affect the runtime of ack. I would checkout the dev branch, make ack-standalone, run timings.pl with --store, and then try on your branch, to see how it compares to what's current.

@pdl
Copy link
Contributor Author

pdl commented Mar 6, 2015

Thanks for the feedback! I'll give the timings script a go. Noting your and @petdance's comments (particularly bugs before features), I'll go and fiddle with it some more and come back with something more refined.

@pdl
Copy link
Contributor Author

pdl commented Mar 9, 2015

@hoelzro, I'm getting values of x_x for all results when I do

perl Makefile.PL
make ack-standalone
dev/timings.pl

Any ideas what I'm doing wrong?

@hoelzro
Copy link
Collaborator

hoelzro commented Mar 9, 2015

@pdl You probably need to check out parrot to your $HOME; the script expects it to be there, and doesn't complain appropriately when it's not. =/

@pdl
Copy link
Contributor Author

pdl commented Mar 9, 2015

Ah, thanks. Assuming you mean https://github.com/parrot/parrot, the timings (as of ea9f26b) look like margin of error difference (I did them in backwards order, sorry, I've adjusted the column heads).

                                    |   1.96 |   2.02 |   2.04 |   2.08 |   2.10 |   2.12 |   2.14 |  max-size |    dev
----------------------------------------------------------------------------------------------------------------------
             ack foo /home/d/parrot |   0.39 |   2.80 |   0.70 |   0.66 |   0.67 |   0.68 |   0.69 |     0.25 |   0.25
        ack foo --cc /home/d/parrot |   0.12 |   1.06 |   0.26 |   0.25 |   0.25 |   0.25 |   0.27 |     0.12 |   0.12
      ack foo --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.14 |   0.14 |   0.14 |   0.16 |     0.10 |   0.10
     ack foo --known /home/d/parrot |   0.39 |   3.16 |   0.77 |   0.83 |   0.64 |   0.64 |   0.65 |     0.24 |   0.24
              ack -f /home/d/parrot |   0.06 |   0.26 |   0.17 |   0.17 |   0.21 |   0.18 |   0.19 |     0.13 |   0.13
         ack -f --cc /home/d/parrot |   0.06 |   0.23 |   0.14 |   0.15 |   0.14 |   0.15 |   0.16 |     0.10 |   0.10
       ack -f --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.15 |   0.14 |   0.15 |   0.16 |     0.10 |   0.11
      ack -f --known /home/d/parrot |   0.06 |   0.59 |   0.34 |   0.36 |   0.17 |   0.18 |   0.19 |     0.13 |   0.13
          ack foo -l /home/d/parrot |   0.58 |   2.77 |   0.33 |   0.33 |   0.34 |   0.34 |   0.25 |     0.19 |   0.19
     ack foo -l --cc /home/d/parrot |   0.20 |   0.80 |   0.18 |   0.19 |   0.18 |   0.18 |   0.17 |     0.11 |   0.11
   ack foo -l --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.14 |   0.14 |   0.15 |   0.16 |     0.10 |   0.10
  ack foo -l --known /home/d/parrot |   0.59 |   3.21 |   0.51 |   0.41 |   0.32 |   0.32 |   0.25 |     0.18 |   0.18
          ack foo -c /home/d/parrot |   0.67 |   2.81 |   0.36 |   0.37 |   0.36 |   0.36 |   0.25 |     0.19 |   0.19
     ack foo -c --cc /home/d/parrot |   0.21 |   0.79 |   0.18 |   0.19 |   0.19 |   0.19 |   0.17 |     0.11 |   0.12
   ack foo -c --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.14 |   0.14 |   0.15 |   0.16 |     0.10 |   0.10
  ack foo -c --known /home/d/parrot |   0.67 |   3.07 |   0.55 |   0.55 |   0.35 |   0.36 |   0.25 |     0.19 |   0.19
        ack foo -A10 /home/d/parrot |   0.50 |   2.94 |   1.41 |   1.34 |   1.36 |   1.35 |   1.39 |     0.96 |   0.95
   ack foo -A10 --cc /home/d/parrot |   0.13 |   0.83 |   0.40 |   0.41 |   0.40 |   0.40 |   0.41 |     0.28 |   0.28
 ack foo -A10 --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.14 |   0.18 |   0.14 |   0.16 |     0.10 |   0.10
ack foo -A10 --known /home/d/parrot |   0.49 |   3.45 |   1.41 |   1.50 |   1.32 |   1.30 |   1.28 |     0.89 |   0.94
        ack foo -B10 /home/d/parrot |   0.56 |   2.83 |   1.43 |   1.43 |   1.43 |   1.42 |   1.45 |     1.08 |   1.08
   ack foo -B10 --cc /home/d/parrot |   0.16 |   0.82 |   0.42 |   0.42 |   0.42 |   0.42 |   0.43 |     0.31 |   0.31
 ack foo -B10 --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.14 |   0.14 |   0.14 |   0.16 |     0.10 |   0.10
ack foo -B10 --known /home/d/parrot |   0.56 |   3.13 |   1.49 |   1.50 |   1.36 |   1.34 |   1.37 |     1.15 |   1.01
        ack foo -C10 /home/d/parrot |   0.62 |   3.03 |   1.66 |   1.62 |   1.62 |   1.65 |   1.64 |     1.09 |   1.09
   ack foo -C10 --cc /home/d/parrot |   0.15 |   0.85 |   0.46 |   0.45 |   0.45 |   0.46 |   0.48 |     0.31 |   0.31
 ack foo -C10 --rust /home/d/parrot |    x_x |   0.23 |   0.14 |   0.14 |   0.14 |   0.14 |   0.16 |     0.10 |   0.10
ack foo -C10 --known /home/d/parrot |   0.61 |   3.47 |   1.70 |   1.75 |   1.52 |   1.56 |   1.54 |     1.01 |   1.03

@hoelzro
Copy link
Collaborator

hoelzro commented Mar 9, 2015

This looks pretty good to me; we're in kind of a code freeze at the moment, but after that's done with, one of us can merge it in.

@petdance
Copy link
Collaborator

petdance commented Mar 9, 2015

What is the use case for --min-file-size? I can't think of why one would use it.

@pdl
Copy link
Contributor Author

pdl commented Mar 9, 2015

More for completeness than anything but it's plausible that when searching directory structure with so many tiny files (like Dancer YAML session files) and a few large interesting files that the little ones become annoying.

@petdance
Copy link
Collaborator

petdance commented Mar 9, 2015

I don't want to add --min-file-size for completeness. I only want it if it will actually get used, and it seems very unlikely to me.

Every bit of code in the project has weight and cost.

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.

3 participants