Clojure

faster, more flexible dispatch for clojure.walk

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code

Description

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, commit 730eb75 in clojure.walk2

Activity

Hide
Vjeran Marcinko added a comment -

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

Show
Vjeran Marcinko added a comment - It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.
Hide
Andy Fingerhut added a comment -

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.

Show
Andy Fingerhut added a comment - 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.
Hide
Alex Miller added a comment -

This patch and ticket are still candidates for future release.

Show
Alex Miller added a comment - This patch and ticket are still candidates for future release.
Hide
Stuart Sierra added a comment -

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

Show
Stuart Sierra added a comment - Added new patch that applies on latest master after CLJ-1105.
Hide
Chouser added a comment -

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.

Show
Chouser added a comment - 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.
Hide
Alex Miller added a comment -

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

Show
Alex Miller added a comment - @Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.
Hide
Alex Miller added a comment -

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

Show
Alex Miller added a comment - @Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.
Hide
Nicola Mometto added a comment -

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

Show
Nicola Mometto added a comment - Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch

People

Vote (6)
Watch (4)

Dates

  • Created:
    Updated: