fn-deps for lambdas #141

Cyrik · 2022-01-02T21:48:15Z

Fixes #51

Uses Clojures internal class cache to also check anonymous function dependencies.

bbatsov · 2022-01-02T22:08:16Z

Small note - we can't really depend on tools.reader, as this might create conflicts with user applications. Orchard has to have no runtime deps.

Cyrik · 2022-01-02T22:23:58Z

Small note - we can't really depend on tools.reader, as this might create conflicts with user applications.

ah thanks, I completely forgot I was still depending on that. I'll rewrite that part to use clojure/read.

Do you have any thoughts on the idea of using the compiler to find the dependencies?

bbatsov · 2022-01-03T06:15:19Z

The idea itself seems reasonable to me.

Cyrik · 2022-01-05T00:03:01Z

I've got a new solution that doesn't need to recompile the code and is a lot faster.
The problem with the new solution is that old classes stay around in clojures cache for some time. I haven't found a way yet to find out which old classes aren't in use anymore.
But the new solution is already a lot better than the old one.
Do you have any ideas on how to fix the final problem?
Also would you want to keep the recompile version as an extra possibility if the actual bytecode is needed for something else?

vemv · 2022-01-05T00:06:01Z

The problem with the new solution is that old classes stay around in clojures cache for some time.

Can you explain this as a PR comment (self-review)? That way we can see the code that the problem in question relates to.

Cyrik · 2022-01-05T00:18:48Z

src/orchard/xref.clj

+  (when-let [^clojure.lang.AFn v (as-val s)]
+    (let [f-class-name (-> v .getClass .getName)
+          field (->> clojure.lang.DynamicClassLoader .getDeclaredFields second)
+          classes (into {} (.get field clojure.lang.DynamicClassLoader))


this holds all functions compiled by clojure:
{...
"dev$dummy"
#object
[java.lang.ref.SoftReference 0x5081395f
"java.lang.ref.SoftReference@5081395f"]
...
}
where lambdas are named "dev$dummy$fn_1234"

the filter uses that fact to just keep the function + lambda classes.

The problem is, when you recompile a function, new lambda classes get generated and the old ones stay around.

the old ones stay around

As in, garbage to be collected?

What causes that retention? This is a defn (and not e.g. a def), so why invoking a defn would retain memory references?

Unless there's global state somewhere, probably your solution is good - it's just minor details like *1 or other repl state that might retain objects for longer than expected.

I probably explained it wrong.
The problem is not the memory, the problem is that the Clojure classloader keeps the old classes in its own cache as java.lang.ref.SoftReference and only deletes that if you try to access the reference after it has been cleaned up.
So if I do the following in my code:

(defn dummy [] (map #(println %) (range 10))) (defn dummy [] (map #(print %) (range 10))) ;;o r just send both versions to the repl by changing the first

classes will now contain "dev$dummy$fn_123" and "dev$dummy$fn_124" where 123 references println and 124 references print.
I haven't found an easy way to get rid of "dev$dummy$fn_123" yet.

Perhaps it would be solved if WeakReference was used instead.

However, as mentioned personally I do not condone classloader hacking :)

This solution does not overwrite the clojure classLoader, so I can't switch the reference type. This just accesses the regular clojure loader through reflection.
I probably shouldn't have kept two separate solutions in the same file, I can see how that is confusing, sorry.

I haven't found an easy way to get rid of "dev$dummy$fn_123" yet.

Does the PR still have this problem?

It might be acceptable to have that problem, if so at least leave a comment explaining what's going on

Yes, the PR still has the problem. I'll play around with it some more to see if I can find a solution. If I can't find one I'll add a comment describing the issue and open a new ticket once the PR is merged.

src/orchard/xref.clj

vemv · 2022-01-05T01:02:28Z

Part of the context is https://lukas-domagala.de/blog/clojure-analysis-and-introspection.html which says:

[clj-kondo] works great but has two new problems:

it's kind of slow (40 seconds for that project, including jars)

does not work for running processes where you don't have the code (remote repls)

Personally I like the middle ground of using clj-kondo but spawn from the same JVM that hosts a given repl.

That way the classpath is always up to date, and one doesn't need external binaries.

clj-kondo itself uses a persistent cache so I don't believe saying it takes 40s to analyze a project is quite representative of day-to-day UX.

As you, I much favor runtime-powered solutions where they make sense. Rich Hickey's hack is a hack, it might work for him because he authored the Clojure compiler.

The compiler's internals are generally not for us to rely on.

So clj-kondo is a fine solution, just as tools.analyzer is (which is very much runtime-powered, so survives any dynamism).

My take would be that unless this PR becomes substantially more minimalistic, it would be not be worth the hassle.

One can always use his own tooling on a personal stack - which I do all the time! Later one can propose PRs as those solutions become more time-proven.

Cyrik · 2022-01-05T02:31:14Z

Thank you for your comments @vemv
I completely understand your position. Clj-kondo is great, it's just a little slow for my use case, which it was not meant to handle.

I've removed the recompile part of the code and commented on the weird IllegalAccess workaround. It's completely fine if you don't want to merge this. This way whoever stumbles over this will have a clean thing to build upon.

vemv

The PR looks more minimalistic now so we can proceed, thanks!

src/orchard/xref.clj

bbatsov · 2022-01-05T05:07:40Z

I've removed the recompile part of the code and commented on the weird IllegalAccess workaround. It's completely fine if you don't want to merge this. This way whoever stumbles over this will have a clean thing to build upon.

Well, our built-in find-usages is not going away, so I'm in favor of improving it. I get @vemv's position but I think it's fine to keep improving what we have given that any alternative will require external deps and extra work that no one has been willing to do for years. :-)

I also think that Clojure's compiler API has been super stable, so the concerns about whether it's a good idea to use its API are probably overdone. Let's drive this PR to the finish line! :-)

src/orchard/xref.clj

Cyrik · 2022-01-06T18:59:42Z

I'm still trying to fix the "old Lamdba" reference problem but otherwise this is done.
There is a hacky fix where you force an OOM Exception on purpose, but it doesn't always work and feels very dirty. Hopefully I'll find another way, or I'll leave it like this.

vemv · 2022-01-06T19:03:04Z

Kudos for the iteration!

There's System/gc, have you tried that? It might make sense to invoke it whenever the user invokes the functionality (as opposed to on a schedule or something similarly invasive).

WIth WeakReference (vs. SoftReference), GCs are even more effective, but I realise we cannot fork Clojure.

Cyrik · 2022-01-07T00:14:25Z

gc does not help, it only clears weakReferences. Softreferences stay around until the memory is needed so badly that an OOM would be triggered.
I've done a few hours of research at this point and could not find any consistent way to clear the old softReferences from the cache. The oom function inside the comment does work sometimes, but it's very hacky and does expand the jvm used memory to the max, so that is not ideal either.
The only other solution I found is JVM TI but using that from clojure would be very annoying and it's not part of every jvm.

TLDR: I think we are 95% of the way to the full solution and the final 5% are either a lot of work or have big drawbacks.

bbatsov · 2022-01-07T08:59:42Z

Might also be a good idea to add a section in the README explaining some of the limitations for the fn-deps/refs. I've been meaning to document all the features of Orchard in the README, but I never got to doing so. We have to start somewhere and that's as good of a place as any.

Cyrik · 2022-01-07T15:04:24Z

I've added some more explanation to the readme and a docstring to the class-cache.
I've linked to my own blog post about the details rather than having too many details in the readme. If you don't like that I can pull out the relevant parts into its own doc file, it might just be too detailed for the main readme.

vemv

Alright, looking good thanks!

Might give it one last look later today.

Could you update the changelog and PR description in the meantime?

Cyrik · 2022-01-07T17:45:39Z

Done.

I also wrote a follow-up post on why the class cache is hard to clear and linked that.

vemv

Almost there 👍 ty!

README.md

src/orchard/xref.clj

Cyrik · 2022-01-08T10:03:56Z

If you think this is relevant to orchard I'll add a fn-transitive-deps function to this PR. It would just be a recursive walk over the dependencies, so it might also be ok for users to write that themselfs.

test/orchard/xref_test.clj

bbatsov · 2022-01-08T18:44:36Z

src/orchard/xref.clj

+  (Math/min 1 2)
+  (def vars (q/vars {:ns-query {:project? true} :private? true}))
+
+  (map fn-deps vars))


The final newline seems to be missing.

bbatsov · 2022-01-08T18:45:17Z

src/orchard/xref.clj

        all-vars (q/vars {:ns-query {:project? true} :private? true})
        deps-map (zipmap all-vars (map fn-deps all-vars))]
    (map first (filter (fn [[_k v]] (contains? v var)) deps-map))))
+
+(comment


Are those some functions you used for test purposes? Might be good idea to add a comment explaining this.

bbatsov · 2022-01-08T18:47:05Z

Looks good to me overall. I even read the blog post - well done! I added a few small remarks and we are good to go. I guess we'll squash this to a single comment in the final version.

Cyrik · 2022-01-08T20:08:02Z

I've added fn-transitive-deps, if you don't want it in here I'll throw it out again or put it in another PR.
Also added one more caveat to the readme. Didn't know this didn't work on AoT code, just found that out because of the transitive test.

test/orchard/xref_test.clj

Co-authored-by: vemv <vemv@users.noreply.github.com>

vemv

One final note from my side

test/orchard/xref_test.clj

vemv · 2022-01-08T21:04:24Z

Cheers 🍻

fn-deps for lambdas

a5301ca

Cyrik changed the title ~~fn-deps for lambdas~~ WIP:fn-deps for lambdas Jan 2, 2022

vemv self-assigned this Jan 2, 2022

vemv marked this pull request as draft January 2, 2022 21:53

vemv removed their assignment Jan 2, 2022

Lukas Domagala added 5 commits January 3, 2022 18:30

fixed reader dependency

93459a0

fix reflection warning

8b09fea

fix kondo and eastwood errors

eccacae

added clojure classLoader cache solution

76bc5d5

illegal access fix

2183ebe

Cyrik commented Jan 5, 2022

View reviewed changes

vemv reviewed Jan 5, 2022

View reviewed changes

src/orchard/xref.clj Outdated Show resolved Hide resolved

vemv reviewed Jan 5, 2022

View reviewed changes

src/orchard/xref.clj Outdated Show resolved Hide resolved

src/orchard/xref.clj Outdated Show resolved Hide resolved

src/orchard/xref.clj Outdated Show resolved Hide resolved

Lukas Domagala added 2 commits January 5, 2022 03:22

removed recompile version

83e642d

clfmt fix

b0eeabc

vemv reviewed Jan 5, 2022

View reviewed changes

src/orchard/xref.clj Outdated Show resolved Hide resolved

src/orchard/xref.clj Outdated Show resolved Hide resolved

src/orchard/xref.clj Outdated Show resolved Hide resolved

cleanup + performance

bc53e14

Cyrik commented Jan 6, 2022

View reviewed changes

src/orchard/xref.clj Show resolved Hide resolved

Cyrik commented Jan 6, 2022

View reviewed changes

src/orchard/xref.clj Outdated Show resolved Hide resolved

added softreference clear attempts

269e673

Cyrik marked this pull request as ready for review January 7, 2022 00:14

Lukas Domagala added 2 commits January 7, 2022 15:59

added documentation

0d28c4f

fix classCache casing

63c8463

vemv reviewed Jan 7, 2022

View reviewed changes

changelog

a67fb75

Cyrik changed the title ~~WIP:fn-deps for lambdas~~ fn-deps for lambdas Jan 7, 2022

vemv reviewed Jan 8, 2022

View reviewed changes

review fixes

d5c8f6d

vemv requested a review from bbatsov January 8, 2022 14:35

vemv approved these changes Jan 8, 2022

View reviewed changes

bbatsov reviewed Jan 8, 2022

View reviewed changes

test/orchard/xref_test.clj Outdated Show resolved Hide resolved

bbatsov reviewed Jan 8, 2022

View reviewed changes

added fn-transitive deps

2689a66

fix clojure 1.8 set access

f0872f4

vemv reviewed Jan 8, 2022

View reviewed changes

test/orchard/xref_test.clj Outdated Show resolved Hide resolved

Update test/orchard/xref_test.clj

d30546b

Co-authored-by: vemv <vemv@users.noreply.github.com>

vemv reviewed Jan 8, 2022

View reviewed changes

test/orchard/xref_test.clj Outdated Show resolved Hide resolved

cleaned up test

f44e878

vemv merged commit 63c5800 into clojure-emacs:master Jan 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fn-deps for lambdas #141

fn-deps for lambdas #141

Cyrik commented Jan 2, 2022 •

edited

Loading

bbatsov commented Jan 2, 2022

Cyrik commented Jan 2, 2022

bbatsov commented Jan 3, 2022

Cyrik commented Jan 5, 2022

vemv commented Jan 5, 2022

Cyrik Jan 5, 2022

vemv Jan 5, 2022

Cyrik Jan 5, 2022

vemv Jan 5, 2022

Cyrik Jan 5, 2022

vemv Jan 5, 2022

Cyrik Jan 5, 2022

vemv commented Jan 5, 2022

Cyrik commented Jan 5, 2022

vemv left a comment

bbatsov commented Jan 5, 2022 •

edited

Loading

Cyrik commented Jan 6, 2022

vemv commented Jan 6, 2022

Cyrik commented Jan 7, 2022

bbatsov commented Jan 7, 2022

Cyrik commented Jan 7, 2022

vemv left a comment

Cyrik commented Jan 7, 2022

vemv left a comment

Cyrik commented Jan 8, 2022

bbatsov Jan 8, 2022

bbatsov Jan 8, 2022

bbatsov commented Jan 8, 2022

Cyrik commented Jan 8, 2022

vemv left a comment

vemv commented Jan 8, 2022

fn-deps for lambdas #141

fn-deps for lambdas #141

Conversation

Cyrik commented Jan 2, 2022 • edited Loading

bbatsov commented Jan 2, 2022

Cyrik commented Jan 2, 2022

bbatsov commented Jan 3, 2022

Cyrik commented Jan 5, 2022

vemv commented Jan 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv commented Jan 5, 2022

Cyrik commented Jan 5, 2022

vemv left a comment

Choose a reason for hiding this comment

bbatsov commented Jan 5, 2022 • edited Loading

Cyrik commented Jan 6, 2022

vemv commented Jan 6, 2022

Cyrik commented Jan 7, 2022

bbatsov commented Jan 7, 2022

Cyrik commented Jan 7, 2022

vemv left a comment

Choose a reason for hiding this comment

Cyrik commented Jan 7, 2022

vemv left a comment

Choose a reason for hiding this comment

Cyrik commented Jan 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jan 8, 2022

Cyrik commented Jan 8, 2022

vemv left a comment

Choose a reason for hiding this comment

vemv commented Jan 8, 2022

Cyrik commented Jan 2, 2022 •

edited

Loading

bbatsov commented Jan 5, 2022 •

edited

Loading