<< Back to previous view

[TNS-44] Reload on a function-by-function basis for improved performance in large projects Created: 24/Jul/16  Updated: 24/Jul/16

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Alex Gunnarson Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: enhancement


 Description   

CURRENT SITUATION

Currently tools.namespace reloads portions of a given Clojure project on a file-by-file (really, namespace-by-namespace) basis. While it works great and performs quite well for most projects — ones which feature namespaces which compile quickly, dependency graphs which don't have too many nodes, and/or dependency graphs which aren't very deep — developers experience a (potentially) significant slowdown the less the project conforms to these three features.

POTENTIAL SOLUTION

A potential solution is this:

  • Walk the forms in all the project's namespaces (significant up-front cost but amortized with incremental reloads)
  • Create a dependency graph of (namespaced, non-local) vars, not just namespaces
  • Detect whether a var's compile-time value (e.g. in the case of a `defn`, its metadata, params, and unevaled body) has changed
  • If so:
  • Reload all the vars which depend on it (according to the dependency graph previously created)
  • Compile it
  • Update the dependency graph
  • Detect whether a var has been added/created
  • If so:
  • Compile it
  • Update the dependency graph

As for tests, it would be nice to re-run only the tests affected by a change to a var, not all the tests in that namespace every time the namespace changes at all.

Essentially, this feature would increase reload granularity (and reload speed) in the same way that namespace-reloading increases granularity (and speed) from recompiling the entire project.



 Comments   
Comment by Alex Gunnarson [ 24/Jul/16 10:27 AM ]

Sorry, the nested lists got messed up. Should be:

  • Walk the forms in all the project's namespaces (significant up-front cost but amortized with incremental reloads)
  • Create a dependency graph of (namespaced, non-local) vars, not just namespaces
  • Detect whether a var's compile-time value (e.g. in the case of a `defn`, its metadata, params, and unevaled body) has changed
    • If so:
      • Reload all the vars which depend on it (according to the dependency graph previously created)
      • Compile it
      • Update the dependency graph
  • Detect whether a var has been added/created
    • If so:
      • Compile it
      • Update the dependency graph




[TNS-43] ns-decl? treats (ns) as valid Created: 28/Mar/16  Updated: 01/Apr/16

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: James Laver Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File ns-decl.patch    
Patch: Code

 Description   

If you try to declare an empty namespace with `(ns)`, you get an ArityException, but ns-decl? does not test for this.

The attached patch simply tests whether it is at least two items long.

I have previously signed the Clojure CA.



 Comments   
Comment by James Laver [ 28/Mar/16 6:45 AM ]

I'm wondering if it's worth updating my patch to also check that the namespace name is a symbol.

Comment by Stuart Sierra [ 01/Apr/16 9:33 AM ]

I feel like I need some more context here. What is the motivation for making a change in this case? Have you encountered real code in which (ns) causes a problem because tools.namespace accepts it as a valid namespace declaration?





[TNS-42] scan-dirs / scan-all can return incorrect dependencies when both clj and cljc files define same namespace Created: 30/Dec/15  Updated: 06/Jan/16

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None
Environment:

tools.namespace 0.2.11 and 0.3.0-alpha2



 Description   

Originally filed as an issue for function scan-all, which at the time I was unaware was deprecated in 0.3.0 alphas. I have tested against the newer scan-dirs as well, and it has the same problem.

When there are both .clj and .cljc files defining the same namespace, tools.namespace scan-dirs and scan-all can use the dependencies from the .cljc file, ignoring those in the .clj file, whereas for Clojure/Java the opposite should be done.

See sample project and its README here for steps to reproduce: https://github.com/jafingerhut/tnstest

Note that Clojure/Java's behavior is to prefer .clj file over a .cljc file defining the same namespace, even if the .clj file is in a directory that is later in the classpath than a .cljc file. According to Alex Miller email on clojure-dev Google group, this behavior is by design. Link: https://groups.google.com/d/msg/clojure-dev/d5Hb1E7zfHE/sPybIAxgCwAJ



 Comments   
Comment by Stuart Sierra [ 05/Jan/16 11:03 AM ]

I can confirm this is a bug.

When I added multiple file extensions, I didn't consider the priority order.

Fixing this may be tricky.

It starts c.t.n.find. c.t.n.find/find-sources-in-dir filters by file extension (controlled by the platform argument in 0.3) but it only considers one directory at a time. Since the files could be in two different directories, it cannot necessarily discover two files with the same name but different extensions.

The next layer is c.t.n.file, which currently doesn't look at file extensions at all, but maybe it should.

In c.t.n.dir we have enough information to prioritize and filter files for the same namespace by extension. But to do it correctly, it has to handle updates too. For example, what happens to a project that has a .cljc file, then adds a .clj file. c.t.n.dir should treat that as removing the .cljc file from the dependency graph.

I think this will require storing the association between a namespace name and a file name, something I'd been hoping to avoid.

Comment by Stuart Sierra [ 05/Jan/16 11:21 AM ]

It might even make sense to make platform a property of the tracker, to make sure the same rules are always used when adding more files.

Comment by Andy Fingerhut [ 06/Jan/16 10:54 AM ]

Agreed that fixing this is not a small change to tools.namespace, hence the reason I don't have a proposed patch already. Came across this while thinking about how Eastwood could/should handle .cljc files. I'll add any proposed patches here if I come up with something.





[TNS-41] Provide warning when AOT enabled for a project Created: 25/Dec/15  Updated: 25/Dec/15

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Zack Maril Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It would be nice if tools.namespace would throw an error when AOT was turned on. I recently turned on AOT for a project and unknowingly broke refresh without thinking. It was during a flurry of other changes and so it took me a while to figure that was the issue and track down that tools.namespace doesn't work with AOT. Is it possible to detect AOT inside of the clojure runtime?






[TNS-37] c.t.n.move: does not support to move .cljc files Created: 05/Sep/15  Updated: 20/Dec/15

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Benedek Fazekas Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-c.t.n.move-add-support-for-.cljc-file-extension.patch     Text File 0001-make-c.t.n.move-platform-aware-guess-platform.patch     Text File 0001-make-c.t.n.move-platform-aware.patch    
Patch: Code

 Description   

it seems that c.t.n.move did not get any reader conditionals love. I use this namespace to power mranderson: a source inlining tool used by cider-nrepl and refactor-nrepl in order to avoid clashes with the code/projects their users work on. lately we started adding dependencies to refactor-nrepl (well, tools.namespace itself) which may use .cljc files. source inlining started failing for these source files.



 Comments   
Comment by Stuart Sierra [ 06/Sep/15 12:57 PM ]

Good idea - glad to see how people are actually using tools.namespace in other dev cools.

Could this be extended further to take 'platform' as a parameter, as in c.t.n.find or c.t.n.dir?

(I deliberately excluded c.t.n.move to limit the scope of changes in 0.3.0, but your patch demonstrates that it may not not be such a difficult change to make.)

Comment by Benedek Fazekas [ 06/Sep/15 3:52 PM ]

first patch does what you asked for I hope. Kept original signiture of move-ns for backward compatibility: clj platform used by default.

second patch is pushing this a bit further: if platform is not provided it tries to guess it based on the file's extension of the old-sym. you might see this as over complication: feel free to pick the first patch then.

Comment by Stuart Sierra [ 06/Nov/15 3:02 PM ]

Thanks for working on these patches. I did some more testing, and uncovered some more complications.

Guessing the file extension leads to unpredictable results, because you don't know which file gets moved. It's not uncommon to have both .clj and .cljs version of the same namespace, for example with :require-macros in ClojureScript.

Unfortunately, even specifying a "platform" argument doesn't completely solve the problem, because you can have .cljc file at the same time as .clj and .cljs. This may not be common, but it's explicitly allowed for in the design of reader conditionals. For example, a library may have a platform-neutral .cljc file and override it with a platform-specific file. In this case, even if you specify a "platform" argument, you still can't control which file gets moved.

So now I think the only way for an operation like "move namespace" to make sense is to move all the files defining that namespace, preserving their file extensions. This seems like what one would want from a user perspective as well.

So move-ns should take new/old symbols, find all the matching files with any extension, move/rename them all while preserving extensions, then apply the textual transformation to all .clj, .cljs, and .cljc files.

If we're searching for files, we might as well eliminate the need to specify the source path, so this also relates to TNS-39.

Comment by Benedek Fazekas [ 20/Dec/15 3:44 PM ]

I have not abandoned this ticket or the patches but it seems it is related to a bigger problem in terms of supporting multiplatform projects therefore related to TNS-38 too.

I am also contemplating the wider context: `move-ns` is not only used in mranderson for source inlining but there is a refactor-nrepl feature (rename file or dir) which basically reimplements this functionality using other parts of TNS.

The mranderson story for me is about exploring different ways of dependency handling really, there was a good lightning talk by Glen Mailer summarising the problem on clojureX this year. My ideas are along creating local, deeply nested dependencies and perhaps enhance the ns macro to be able to load them – not even sure that is feasible. This would eliminate the need for source inlining and more importantly would be much more hygienic approach I think.

Hope this makes sense. Sorry for the brain dump which is obviously exceeds the scope of this ticket.





[TNS-39] Single classpath argument for c.t.n.move Created: 06/Nov/15  Updated: 06/Nov/15

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None


 Description   

clojure.tools.namespace.move/move-ns requires that the caller provide both 1) the directory containing the file to be moved; and 2) the collection of directories containing all source files to be updated with the new name. This is redundant.

Instead, we can search for the file in all source directories, move it within the same root directory, then update all files in all directories.

In addition, with java.classpath in TNS-36, the set of directories to search can default to directories on the classpath. This will be much more convenient for REPL use.

This change can be made without breaking older arities of move-ns.






[TNS-6] Attempt to reload deleted file Created: 14/Dec/12  Updated: 12/Aug/15

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 1
Labels: None


 Description   

I can't identify the exact circumstances, but I have seen events where a source code file has been deleted but clojure.tools.namespace.repl/refresh still tries to reload it. Because the file doesn't exist, there's an exception when you try to load it, so you're stuck.



 Comments   
Comment by Gary Fredericks [ 12/Aug/15 10:21 AM ]

This happens to me pretty frequently, especially when switching branches (which is ironically the best use case for calling refresh).

Comment by Stuart Sierra [ 12/Aug/15 10:24 AM ]

I still don't know exactly how this occurs.

The workaround for now is to call c.t.n.repl/clear, added in 0.2.5

Comment by Gary Fredericks [ 12/Aug/15 1:26 PM ]

yep, noticed that independently and just confirmed that it works.





[TNS-24] Broken tracker after mis-named namespace Created: 07/Sep/14  Updated: 11/Sep/14

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A tools.namespace dependency tracker can get into an inconsistent state after attempting to load a file with a namespace declaration that does not match the file's name.

Steps to reproduce:

In a new Clojure project, create a file foo.clj on the classpath containing the namespace declaration (ns wrong-ns-declaration-for-foo)

Then at the REPL:

user=> (use 'clojure.tools.namespace.repl)
nil
user=> (refresh)
:reloading (wrong-ns-declaration-for-foo)
:error-while-loading wrong-ns-declaration-for-foo
#<FileNotFoundException java.io.FileNotFoundException: Could not locate wrong_ns_declaration_for_foo__init.class or wrong_ns_declaration_for_foo.clj on classpath: >

Edit the file foo.clj so that its namespace declaration is correct: (ns foo)

But at the REPL, refresh still doesn't work:

user=> (refresh)
:reloading (foo wrong-ns-declaration-for-foo)
:error-while-loading wrong-ns-declaration-for-foo
#<FileNotFoundException java.io.FileNotFoundException: Could not locate wrong_ns_declaration_for_foo__init.class or wrong_ns_declaration_for_foo.clj on classpath: >

Since tools.namespace 0.2.5, the workaround is to call clear:

user=> (clear)
{}
user=> (refresh)
:reloading (foo)
:ok


 Comments   
Comment by Andy Fingerhut [ 11/Sep/14 6:38 PM ]

Such files introduce problems for all kinds of tools, not just tools.namespace. I don't know all of the consequences, but two are listed here: https://github.com/jonase/eastwood#check-consistency-of-namespace-and-file-names

I have added warnings in Eastwood whenever it finds files like this, and doesn't do any linting checks on any files until such things are corrected.





[TNS-7] Stack overflow on refresh after circular dependency detected Created: 17/Apr/13  Updated: 17/Apr/13

Status: Open
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 1
Labels: None
Environment:

tools.namespace 0.2.3



 Description   
user> (refresh)
Exception Circular dependency between com.example.foo and com.example.bar  clojure.tools.namespace.dependency.MapDependencyGraph (dependency.clj:74)
user> (refresh)
StackOverflowError   clojure.lang.PersistentHashMap$BitmapIndexedNode.index (PersistentHashMap.java:576)
user> (clojure.repl/pst)
StackOverflowError 
	clojure.lang.PersistentHashMap$BitmapIndexedNode.index (PersistentHashMap.java:576)
	clojure.lang.PersistentHashMap$BitmapIndexedNode.find (PersistentHashMap.java:676)
	clojure.lang.PersistentHashMap$ArrayNode.find (PersistentHashMap.java:396)
	clojure.lang.PersistentHashMap.valAt (PersistentHashMap.java:152)
	clojure.lang.PersistentHashMap.valAt (PersistentHashMap.java:156)
	clojure.lang.RT.get (RT.java:645)
	clojure.tools.namespace.dependency/transitive (dependency.clj:52)
	clojure.tools.namespace.dependency/transitive/fn--7043 (dependency.clj:51)
	clojure.core.protocols/fn--6022 (protocols.clj:79)
	clojure.core.protocols/fn--5979/G--5974--5992 (protocols.clj:13)
	clojure.core/reduce (core.clj:6177)
	clojure.tools.namespace.dependency/transitive (dependency.clj:52)
nil





Generated at Tue Aug 30 04:28:09 CDT 2016 using JIRA 4.4#649-r158309.