Performance Optimization. Non-cropped Status-Map. Dependency Cleanup. Path-Trie + Cached executed.#12
Conversation
…zations, execute config map, updated docs and tests BREAKING execute 3rd arg is now config map instead of binding. Removed :all-inside-recur and :point-and-all-inside-recur dependency modes. Removed print-stats/print-trace from utils. Changed execute-trace signature. ADDED commando.debug namespace — execute-debug with 6 display modes, execute-trace for nested call tracing. ADDED commando.impl.pathtrie — trie for O(depth) command lookup, built during find-commands pass. ADDED status-map keys: :internal/cm-results, :internal/path-trie, :internal/original-instruction. OPTIMIZED find-commands (transient queue + set), execute-commands (transient results, index loop), build-dependency-graph (trie-based, transients), topological-sort (transient maps/queue), CommandMapPath (cached hash), Malli validation (pre-computed validators). FIXED execute-command-impl guard for non-map command-data. Fixed point dependency error data. UPDATED resolve-relative-path and find-anchor-path refactored to loop. Updated README, docs, tests split and .cljc conversion.
…ning about replacemnt for deftype constructor ->CommandMapPath
There was a problem hiding this comment.
Generally really awesome change! A lot of good stuff!
I was thinking about the trade off, between code readability and performance optimization.
Because it gets harder to read it, but for our library performance > readability.
But we can do as much as we can to ensure that this code is written there is for optimization, so next time there is change there it is important to test how it impacts performance and so that it does not get lost.
So it would be beneficial to add in all of those places a inline comment explaining this was done for optimization of performance. Because that knowledge gets lost without it.
and [100% optional] what would be super extra if it's possible is to add tests to that specific function that checks that performance in this key area is kept. Even if it's to be manually run with other performance tests, this would be a nice addition to tests you improved in this PR regarding general performance.
| (if (-validator:command-fn m) | ||
| true | ||
| (malli-error/humanize (-explainer:command-fn m)))) |
There was a problem hiding this comment.
Very interesting that moving that to malli/validator has such a performance impact
| (if (= prefix-len 0) | ||
| true | ||
| (if (< s-len prefix-len) | ||
| false | ||
| (loop [i 0] | ||
| (if (= i prefix-len) | ||
| true | ||
| (if (= (nth s i) (nth prefix i)) | ||
| (recur (inc i)) | ||
| false)))))))) |
There was a problem hiding this comment.
I understand this is for performance reasons, but can we move this part to separate function with clear name?
Previously this line was very clear when you read it, now you need to analyze it to understand what's this about and why.
| ^{:doc | ||
| "Represents a command found in the instruction map at a specific `path`. | ||
| Holds both the path to the command and its command specification data. | ||
| Hash is cached at construction time for fast map/set lookups. |
| (let [cmd-count (count commands)] | ||
| (loop [current-instruction instruction | ||
| idx 0 | ||
| results (transient {})] |
There was a problem hiding this comment.
That's cool about the transient, never had a need to use it.
Great that the code is more efficient now, but the ease to understand this code has gone significantly down :(.
| (utils/format-time duration)]))) | ||
|
|
||
| (def ^:private -coercer:status-map-message | ||
| (malli/coercer [:map [:message [:string {:min 5}]]])) |
| :registry))) | ||
|
|
||
| ;; -- Execute -- | ||
| ;; -- Full Execute (internal) -- |
There was a problem hiding this comment.
nit pick, a bit verbose, to say full execute 3 times in 3 lines, keep it dry :D
| :error-data-string - (boolean) serialize exception data as strings | ||
| :hook-execute-start - (fn [status-map]) called before execution | ||
| :hook-execute-end - (fn [status-map]) called after execution |
| (let [r (debug/execute-trace | ||
| #(commando/execute | ||
| [builtin/command-fn-spec | ||
| builtin/command-from-spec | ||
| builtin/command-macro-spec | ||
| builtin/command-mutation-spec] | ||
| {:value {:commando/mutation :rand-n :v 200} | ||
| :result {:commando/macro :sum-n | ||
| :v {:commando/from [:value]}}}))] | ||
| [builtin/command-fn-spec | ||
| builtin/command-from-spec | ||
| builtin/command-macro-spec | ||
| builtin/command-mutation-spec] | ||
| {:value {:commando/mutation :rand-n :v 200} | ||
| :result {:commando/macro :sum-n | ||
| :v {:commando/from [:value]}}})] |
Breaking Changes
BREAKING
executeaccepts an optional third argument — a config map that replaces the oldbinding-based*execute-config*dynamic var for passing options. Users no longer need(binding [utils/*execute-config* {...}] (execute ...)).(execute registry instruction)— unchanged(execute registry instruction {:error-data-string false})— new opts map:error-data-string,:hook-execute-start,:hook-execute-endBREAKING
execute-tracesignature changed from(execute-trace exec-fn)to(execute-trace registry instruction)/(execute-trace registry instruction opts). No longer requires wrapping in a zero-arg function.BREAKING Removed
:debug-resultconfiguration option. Internal structures (:internal/cm-list,:internal/cm-dependency,:internal/cm-running-order,:internal/path-trie,:internal/cm-results,:internal/original-instruction,:registry) are now always retained in the status-map. If you relied on stripped status-maps, dissoc internal keys yourself.BREAKING Removed dependency modes
:all-inside-recurand:point-and-all-inside-recur. Supported modes::point,:all-inside,:none.BREAKING Removed
print-stats,print-trace(print-deep-stats) fromcommando.impl.utils. Replaced bycommando.debugnamespace.BREAKING
registry_test.cljrenamed toregistry_test.cljc(cross-platform).BREAKING status-map return result as is
Added
ADDED
commando.debugnamespace — dedicated module for debug visualization:execute-debug— execute and visualize in one of six display modes::tree,:table,:graph,:stats,:instr-before/:instr-after. Supports combining multiple modes via vector.execute-trace— trace all nestedcommando/executecalls with timing and structure.ADDED
commando.impl.pathtriemodule — trie data structure for O(depth) command lookup by path. Built during the same traversal pass as command discovery, eliminating extra passes over the instruction tree.ADDED new status-map keys always present after execution:
:internal/cm-results— map{CommandMapPath -> resolved-value}with result of each command's:applyfunction.:internal/path-trie— nested trie for efficient command lookup by path.:internal/original-instruction— the original instruction before command evaluation.ADDED
structural-command-type?andstructural-command-typesincommando.impl.registryfor detecting internal structural commands (:instruction/_value,:instruction/_map,:instruction/_vec).Performance
OPTIMIZED
find-commandsBFS traversal incommando.impl.finding_commands:enqueue-coll-children!/enqueue-command-children!instead of intermediate mapv vectors.OPTIMIZED
execute-commandsincommando.impl.executing:nthinstead ofrest/firston remaining commands.OPTIMIZED
build-dependency-graphincommando.impl.dependency:find-commandsinstead of rebuilding it.:all-insidedependency resolution usesreduce-kvon trie subtree instead of dissoc+vals+keep+set chain.OPTIMIZED
topological-sortincommando.impl.graph:OPTIMIZED
CommandMapPathincommando.impl.command_map:vector-starts-with?uses indexed loop instead of lazy seq/take.OPTIMIZED Malli validation in
commando.commands.builtin:Fixed
FIXED
execute-command-implincommando.impl.executing— guard for non-mapcommand-databefore callingdissocon driver keys (:=>,"=>).FIXED point dependency errors in
commando.impl.dependencynow include:command-path,:path, and:commandin error data.Updated
UPDATED
resolve-relative-pathincommando.impl.dependency— refactored from reduce to recursive loop for clarity and correct early termination.UPDATED
find-anchor-pathincommando.impl.dependency— refactored from reduce to loop.UPDATED documentation — restructured
README.mdwith comprehensive status-map documentation, improved navigation, "Managing the Registry" and "Debugging" sections. Moved doc files toexamples/with runnable code examples.UPDATED performance test alias from
:performanceto:performance-coreindeps.edn.UPDATED tests — split monolithic
core_test.cljcinto focused namespaces:dependency_test.cljc,finding_commands_test.cljc,graph_test.cljc,pathtrie_test.cljc. Addeddebug_test.cljc. Convertedregistry_testto.cljcfor cross-platform support.