Welcome! Please see the About page for a little more info on how this works.

+5 votes
in Clojure by

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the {{walk}} protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, (link: https://github.com/stuartsierra/clojure.walk2/commit/730eb756dcc424aa535d0872bd626079955c3892 text: commit 730eb75 in clojure.walk2)

10 Answers

0 votes
by

Comment made by: vmarcinko

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

0 votes
by

Comment made by: jafingerhut

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

0 votes
by

Comment made by: alexmiller

This patch and ticket are still candidates for future release.

0 votes
by

Comment made by: stuart.sierra

Added new patch that applies on latest master after CLJ-1105.

0 votes
by
_Comment made by: chouser@n01se.net_

The way this patch behaves can be surprising compared to regular maps:


(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}


Note how the {{[:a 1]}} entry is removed from the map, but not from the record.

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then {{walk}} could take advantage of that.
0 votes
by

Comment made by: alexmiller

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

0 votes
by

Comment made by: alexmiller

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on. :)

0 votes
by

Comment made by: bronsa

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch

0 votes
by

Comment made by: glts

The current implementation of clojure.walk does not preserve metadata.

If this new implementation is ever revisited, it would be great to also keep the metadata on the elements where possible.

0 votes
by
Reference: https://clojure.atlassian.net/browse/CLJ-1239 (reported by stuart.sierra)
...