-
Notifications
You must be signed in to change notification settings - Fork 1
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
PERS-13 Optimization #15
Conversation
Criterium Benchmarks:
Note: Tested with REPL reset on second benchmark.
|
looks good to me, nice optimization! |
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.
To the extent I understand the domain this looks really good and contains some substantial refactoring and optimization.
I didn't fully grok what the macro I commented on there is doing, but it seems similar to what clojure.spec.alpha
does when you use a predicate function as a spec, where it returns the var name of the function on error.
## 0.5.0 - 2021-05-25 | ||
- Update persephone API function names and optional args to be in line with each other. | ||
- Statement Templates are no longer compiled on each Statement read. | ||
- Compiled JSONPaths are now stored in a stateful cache for quick access. | ||
- Optimized validation function creation: | ||
- Presence colls are now turned into sets during compilation. | ||
- Removed use of spec and `explain-data`. | ||
- Optimized FSM creation: | ||
- Removed pattern matching from `pattern-validation/pattern->fsm`. | ||
- Removed `fsm/move-nfa` as a standalone function. | ||
- `fsm/epsilon-closure` now uses transients internally. | ||
- Optimized Pathetic dep (see [api-refactor](https://github.com/yetanalytics/pathetic/pull/3)). | ||
|
||
## 0.4.0 - 2021-02-25 | ||
- Added FSM specs and generative tests in the `gen` namespace. | ||
- Added DATASIM tests in the `gen` namespace to test API functions on statement streams. | ||
- Generative tests have their own aliases in `deps.edn`. | ||
- Modified `match-next-statement` to handle multiple Patterns and Pattern outputs. |
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.
This is an insanely great changelog entry. We don't normally have the discipline to maintain these (as I'm sure you've noticed), except in limited cases where it's part of our contract, but I'd like to see them become more common
(defmacro wrap-pred | ||
"Wrap a predicate function f such that if f returns true, | ||
return nil, and if f returns false, return the keywordized | ||
name of f." | ||
[f] | ||
(assert (symbol? f)) | ||
(let [pred-name# (keyword f)] | ||
`(fn [x#] (if (~f x#) nil ~pred-name#))))) |
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.
So this allows you to take a collection of predicates, use this on each and then run them all on something and know (by function name) which specific ones are false?
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.
Basically yes. And as you can see by how it's use in the create-pred functions, the macros allow for predicates to be transformed this way via conditional threading.
It is indeed the same as what spec does - returns the function name on failure - but without all the extra overhead that was a source of unoptimized performance.
Optimized Persephone
explain-data
.pattern-validation/pattern->fsm
.fsm/move-nfa
as a standalone function.fsm/epsilon-closure
now uses transients internally.persephone
API function names and optional args to be in line with each other.