ClojureScript

CLJS Analyzer does not correctly detect cache hits for analyzed spec files

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    OS X
  • Patch:
    Code and Test

Description

When Analyzing a cljs file which contains only (cljs.spec.alpha/def ...) forms and no clojure core (def ...) special forms, the Analyzer does not determine the source file to have been analyzed on https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L4021. When this is combined with the :cache-analysis true compiler option, it results in the analysis cache continually being repopulated from disk, and in certain degenerate cases, poor compilation performance.

Activity

Hide
Alexander Redington added a comment -

Steps to reproduce: have multiple cljs files require one namespace that contains only (s/def ...) forms, such that analysis is examined multiple times for the spec namespace. Run with compiler-options :verbose true and :cache-analysis true, and observe the file is analyzed multiple times and the cache is re-read multiple times.

Workarounds: Add a meaningless (def ...) form to the files which are repeatedly analyzed, and they will suddenly stop being analyzed more than once.

On my local machine doing this with the worst offendeding namespaces dropped compile times from 104 seconds to 34 seconds.

Show
Alexander Redington added a comment - Steps to reproduce: have multiple cljs files require one namespace that contains only (s/def ...) forms, such that analysis is examined multiple times for the spec namespace. Run with compiler-options :verbose true and :cache-analysis true, and observe the file is analyzed multiple times and the cache is re-read multiple times. Workarounds: Add a meaningless (def ...) form to the files which are repeatedly analyzed, and they will suddenly stop being analyzed more than once. On my local machine doing this with the worst offendeding namespaces dropped compile times from 104 seconds to 34 seconds.
Hide
Mike Fikes added a comment -

There are a couple of places in the code that interpret the presence of defs in the analysis cache as implying that the namespace has been analyzed. (One is the spot pointed out by Alex, and the other is https://github.com/clojure/clojurescript/blob/2389e52049a9bd001d173a1cb4772ed8a25de196/src/main/clojure/cljs/analyzer.cljc#L2120 )

The attached patch essentially forces the issue, ensuring that there is at least an empty defs map upon successful analysis. (An alternative approach would be to revise the AST slightly by introducing an explicit :analyzed true key-value.)

Note that, in order to test with the attached patch, you'd need to ensure that the patch in CLJS-2405 is applied as well, as it affects proper cache loading, especially in the case where specs are involved.

Show
Mike Fikes added a comment - There are a couple of places in the code that interpret the presence of defs in the analysis cache as implying that the namespace has been analyzed. (One is the spot pointed out by Alex, and the other is https://github.com/clojure/clojurescript/blob/2389e52049a9bd001d173a1cb4772ed8a25de196/src/main/clojure/cljs/analyzer.cljc#L2120 ) The attached patch essentially forces the issue, ensuring that there is at least an empty defs map upon successful analysis. (An alternative approach would be to revise the AST slightly by introducing an explicit :analyzed true key-value.) Note that, in order to test with the attached patch, you'd need to ensure that the patch in CLJS-2405 is applied as well, as it affects proper cache loading, especially in the case where specs are involved.
Hide
Mike Fikes added a comment -

Another somewhat related issue to watch out for, even if this one is fixed: CLJS-2417

Show
Mike Fikes added a comment - Another somewhat related issue to watch out for, even if this one is fixed: CLJS-2417

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: