maskgen: --mintime/--mincomplexity act as maximums, and ?b/?h/?H/?1-?4 masks are costed as 1 - #10
Open
bandrel wants to merge 1 commit into
Open
maskgen: --mintime/--mincomplexity act as maximums, and ?b/?h/?H/?1-?4 masks are costed as 1#10bandrel wants to merge 1 commit into
bandrel wants to merge 1 commit into
Conversation
…eporting maskgen --mintime and --mincomplexity compared with <=, so both behaved as maximums and selected exactly the masks the operator was excluding. getcomplexity had lost ?b, ?h, ?H and ?1-?4 support along with the --custom-charset*-len options, so masks using them were costed as complexity 1: they looked instant, sorted to the front and never counted against --targettime. Uncostable masks are now rejected rather than silently understated. A mask appearing in more than one input file was overwritten rather than accumulated while the coverage denominator counted every occurrence, so a complete mask set could report a fraction of its true coverage. Mask runtimes used integer division, so anything under one second became 0s -- at the default 1e9 keys/sec that is every mask below a billion candidates, which defeated --targettime and the time filters. Also: - --targettime withholds the mask that would overshoot instead of writing it and then reporting the overshoot. - Zero-valued filters are honoured rather than discarded as falsey, and --pps 0 is rejected instead of dividing by zero. - statsgen's Advanced Masks section ignored --hiderare and always hid entries under 1%; it is now gated like every other section, and the output file still receives the full set. - Empty or fully filtered input no longer divides by zero. - statsgen closes its output file on every exit path. Regression tests are in https://github.com/bandrel/pack.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two of these are regressions against
iphelix/pack@622dd92, so they affect thisfork specifically.
--mintimeand--mincomplexityare maximumsUpstream has
>=for both. As written, asking for a minimum silently returnsexactly the masks you were trying to exclude.
minoccurrenceandminlengthkept their
>=, so only these two are affected.?b,?h,?Hand?1-?4are costed as complexity 1getcomplexityhandles onlyl u d s a. Upstream622dd92handlesl u d s a b h H 1 2 3 4(added in "Added the rest of the built-in charsets"and "Added support for custom character sets"), and the
--custom-charset*-lenoptions are gone from the CLI here entirely.
The consequence is quiet and expensive: a hex mask like
?h?h?h?h?h?h?h?h?h?hfalls to the
elsebranch, which prints a warning but leavescountunmultiplied. The mask is costed as 1, so it looks instant, sorts to the front
of an
--optindexrun, and never counts against--targettime. With this PRthat mask correctly costs 16^10 and reports ~18 minutes at 1e9 keys/sec.
Uncostable masks (a literal in a
.hcmask, or?1with no declared length) arenow rejected with a message rather than admitted with an understated cost.
Masks repeated across input files are not aggregated
self.masks[mask] = dict()is last-write-wins, but every occurrence is added tototal_occurrence. Loading two files that share masks understates coverage —easy to see with the same file twice, which reported
Masks coverage: 50% (6/12)before and100% (12/12)after.Runtimes truncate to zero
mask_complexity // self.ppsmeans anything under a second reports0s. At thedefault 1e9 keys/sec that is every mask below a billion candidates:
?d?d?d?d?d?d?d?d(1e8) and?l?l?l?l?l?l(3.1e8) both showed0:00:00. Thatmakes
--mintime/--maxtimeunable to distinguish them and lets--targettimeaccumulate zeros without ever tripping. Complexity stays integraland the division is now floating point, which also resolves the
# TODO: Something wrong here, complexity and time doesn't match with estimated from policygen—getmaskscoveragesummed complexity and divided once whileloadmasksdivided per mask, so the two paths genuinely disagreed.Smaller items
--targettimewrote the overshooting mask to the output file before breaking,so the emitted set always exceeded the budget by one mask.
if options.mintime:and friends discard a legitimate0;--pps 0wasaccepted and then divided by. Now
is not Nonethroughout, and--pps 0isrejected.
statsgen's Advanced Masks section hardcodedif count*100//filter_counter > 0while the other three sections gate onself.hiderare, so sub-1% advancedmasks were always hidden from the display regardless of the flag. The output
file was and remains complete.
statsgennever closed its output file, so a killed run could leave atruncated
.masks.Testing
Regression tests for each item are in
bandrel/pack as
tests/test_pack.py. Happyto split this into per-bug PRs if you'd rather review them separately.
Related: #9 (rulegen), iphelix#31 (the subset affecting the
Python 2 original).