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

Validate fixes #26

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Stantheman
Copy link

This change makes st a bit quicker for large inputs. The main boost comes from de-duplicating the validate check for every number. The validation check is also replaced with Scalar::Util's looks_like_number, which passes the existing test suite, is part of core, and is implemented in C.

This change, as it is now, would require a version bump on the library since previously, process would die on invalid input. This change assumes the caller has validated the input. It should be straightforward to avoid that if necessary.

I've included some simple runs on a 40mb file showing the before and after below. The run time for this file was reduced to 14s, originally taking 21s. Both times were taken after multiple runs of tests to account for page caches being warm etc.

Current st:

-bash-4.2$ time st --complete /tmp/input.txt
N	min	q1	median	q3	max	sum	mean	stddev	stderrvariance
4.45998e+06	0.002401	0.00371	0.004048	0.004653	0.069798	28189.6	0.00632058	0.00852295	4.03574e-06	7.26406e-05

real	0m21.341s
user	0m20.551s
sys	0m0.790s

Proposed:

-bash-4.2$ time st --complete /tmp/input.txt
N	min	q1	median	q3	max	sum	mean	stddev	stderrvariance
4.45998e+06	0.002401	0.00371	0.004048	0.004653	0.069798	28189.6	0.00632058	0.00852295	4.03574e-06	7.26406e-05

real	0m14.079s
user	0m13.255s
sys	0m0.823s

Passes the same suite of tests that exists now, removes custom regex
The st script validates each number, then process validates again, which
adds time to the script for large inputs
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.

1 participant