Clojure

Improve `refer` performance

Details

  • Type: Enhancement Enhancement
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: Release 1.6
  • Fix Version/s: Release 1.11
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Vetted

Description

refer underlies require, use, and refer-clojure use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

Approach: Some improvements that can be made:

  • Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
  • Use transients to build map of references to refer
  • Instead of cas'ing each new reference individually, build map of all changes, then cas
  • For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

Performance:

expr in a new repl 1.7.0-beta3 1.7.0-beta3+patch
(in-ns 'foo) (clojure.core/refer 'clojure.core) 2.65 ms 0.994 ms
(in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) 1.04 ms 0.113 ms
(use 'criterium.core) 0.877 ms 0.762 ms
(require '[clojure.core.async :refer (>!! <!! chan close!)]) 3408 ms 3302 ms

Patch: clj-1730-2.patch

Screening Notes

Patch appears correct but we should consider:

  • non-idiomatic use of if instead of when makes branches hard to read
  • non-idiomatic indentation of if (both branches on one line) hard to read
  • I don't think not found should be an IllegalAccessError, but the original code did this already, so ...
  • the optimistic concurrency loop around the swap will never give up, is this ok?
  1. clj-1730-2.patch
    07/Sep/17 10:52 AM
    8 kB
    Alex Miller
  2. refer-perf.patch
    13/May/15 11:52 AM
    7 kB
    Alex Miller
  1. Screenshot from 2018-08-01 23-23-41.png
    208 kB
    01/Aug/18 10:30 PM

Activity

Alex Miller made changes -
Field Original Value New Value
Labels performance
Alex Miller made changes -
Approval Triaged [ 10120 ]
Alex Miller made changes -
Description {{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (use 'criterium.core)" | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


{{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements here are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (use 'criterium.core)" | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


Alex Miller made changes -
Description {{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements here are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (use 'criterium.core)" | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


{{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core)" | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


Alex Miller made changes -
Description {{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core)" | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


{{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core) | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


Alex Miller made changes -
Labels performance ft performance
Alex Miller made changes -
Approval Triaged [ 10120 ] Prescreened [ 10220 ]
Stuart Halloway made changes -
Description {{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core) | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch


{{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core) | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch

*Screening Notes*

Patch appears correct but we should consider:

* non-idiomatic use of {{if}} instead of {{when}} makes branches hard to read
* non-idiomatic indentation of {{if}} (both branches on one line) hard to read
* I don't think not found should be an {{IllegalAccessError}}, but the original code did this already, so ...
* the optimistic concurrency loop around the swap will *never* give up, is this ok?
Alex Miller made changes -
Description {{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core) | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* refer-perf.patch

*Screening Notes*

Patch appears correct but we should consider:

* non-idiomatic use of {{if}} instead of {{when}} makes branches hard to read
* non-idiomatic indentation of {{if}} (both branches on one line) hard to read
* I don't think not found should be an {{IllegalAccessError}}, but the original code did this already, so ...
* the optimistic concurrency loop around the swap will *never* give up, is this ok?
{{refer}} underlies {{require}}, {{use}}, and {{refer-clojure}} use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

*Approach:* Some improvements that can be made:
- Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
- Use transients to build map of references to refer
- Instead of cas'ing each new reference individually, build map of all changes, then cas
- For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

*Performance:*

|| expr in a new repl || 1.7.0-beta3 || 1.7.0-beta3+patch ||
| (in-ns 'foo) (clojure.core/refer 'clojure.core) | 2.65 ms | 0.994 ms |
| (in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) | 1.04 ms | 0.113 ms |
| (use 'criterium.core) | 0.877 ms | 0.762 ms |
| (require '[clojure.core.async :refer (>!! <!! chan close!)]) | 3408 ms | 3302 ms |

*Patch:* clj-1730-2.patch

*Screening Notes*

Patch appears correct but we should consider:

* non-idiomatic use of {{if}} instead of {{when}} makes branches hard to read
* non-idiomatic indentation of {{if}} (both branches on one line) hard to read
* I don't think not found should be an {{IllegalAccessError}}, but the original code did this already, so ...
* the optimistic concurrency loop around the swap will *never* give up, is this ok?
Attachment clj-1730-2.patch [ 17328 ]
Rich Hickey made changes -
Fix Version/s Release 1.10 [ 11451 ]
Approval Prescreened [ 10220 ] Vetted [ 10003 ]
Ghadi Shayban made changes -
Alex Miller made changes -
Fix Version/s Release 1.10 [ 11451 ]
Fix Version/s Release 1.11 [ 11750 ]

People

Vote (3)
Watch (4)

Dates

  • Created:
    Updated: