-
Notifications
You must be signed in to change notification settings - Fork 99
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
Improve extensibility of Zxcvbn and Matching and optimize memory usage #44
Conversation
Use singleton empty collections where possible, prefer conversions from an array to a list avoiding array copy and prefer HashMap copy constructor to manual copy.
Previously the only way how to provide custom dictionaries was by using `sanitizedInputs` argument of `Zxcvbn#measure(String, List<String>)` method. The disadvantage was that the custom dictionary was reprocessed for every call and for large custom dictionary it could incur considerable overhead. To improve the situation, Zxcvbn class is modified to allow supplying custom instance of Matching class and also Matching class is changed to allow the needed level of extensibility. This extensibility changes maintain full backward compatibility.
@psvo Thanks for great PR. I will chk it ASAP! |
@psvo I would appreciate your reply to my comment. |
@vvatanabe Hi, I'm sorry, but I don't see any comment, where you asked me something. Can you point me in the right direction? Thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psvo
I have a question. Why did you change from array to list?
} | ||
|
||
public List<Match> omnimatch(String password) { | ||
return new OmnibusMatcher(rankedDictionaries).execute(password); | ||
} | ||
|
||
private static Map<String, Integer> buildRankedDict(String[] orderedList) { | ||
protected static Map<String, Integer> buildRankedDict(List<String> orderedList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psvo
I have a question. Why did you change from array to list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we need to pass an array only when building system dictionaries. Using Arrays.asList
does not copy underlining array, just wraps it. For orderedList (user inputs), we natively have a list. So I wanted to avoid copying it to an array. This way we save array allocation and avoid one iteration over orderedList in the hot path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psvo I got it! I will release a new version ASAP. I appreciate your works!
Change
Zxcvbn
andMatching
classes to make extending their behavior possible without changing current public API. The main idea is to allow usage of custom (extended)Matching
class withZxcvbn
.One of possible uses is implementing support for custom user dictionary that is parsed/processed just once and then reused for subsequent calls to
Zxcvbn#measure(String, List<String>)
. Passing large user dictionary to each call of measure causes noticeable slowdown and generates unnecessary GC overhead.Also includes optimizations avoiding unnecessary allocations for standard use cases.