search

clojure-emacs / orchard

A fertile ground for Clojure tooling
Eclipse Public License 1.0
323 stars 54 forks source link

[clojuredocs] Protect cache filling with a lock #239

Closed alexander-yakushev closed 2 months ago

alexander-yakushev commented 2 months ago

While profiling the test runs, I noticed that the function that parser Clojuredocs export spends significant time within pmap invocation. Looks like multiple threads are racing to compute the cache.

The change is unlikely to impact users anyhow, unless there is a situation with a slow machine like there was in Compliment and its own caching. In any case, this change wouldn't hurt.

vemv commented 2 months ago

Seems a reasonable addition!

You might want to add similar locks throughout the ns

alexander-yakushev commented 2 months ago

You might want to add similar locks throughout the ns

Right, I see now that there is another place where stuff gets loaded. Let me rethink and refactor this a bit.

alexander-yakushev commented 2 months ago

Tests are passing now.

vemv commented 2 months ago

This seems reasonable to merge although my humble suggestion would be to make this as 'foolproof' as possible - these sections of code can get updated once every couple years so something simple would probably be appreciated - especially as we're dealing with locking.

alexander-yakushev commented 2 months ago

I don't think that serializing all access to cache (including reads) is necessary. I'll try to expand the comment so that future editors understand better what is going on.

vemv commented 2 months ago

I didn't suggest serializing reads - instead I wanted to prevent any maintainers from having to reason much - energies are a pretty scarce resource when there are 100 issues open at any given time.

A readwrite lock might help. And/or encapsulating things in some or other way.

But if you don't see a clear improvement that's fine too - no issue!

alexander-yakushev commented 2 months ago

Given that I'm the first person to take a look at this namespace in 4 years, I can say yes, the maintainers' energies are safe here. 😆

Just a joke, no offense. I think this PR is good to merge.

vemv commented 2 months ago

What I meant is, next time I or someone else has to touch this, maybe 4 years from now, will we scratch our heads?

vemv commented 2 months ago

Either way, it's no big deal - simply trying to suggest what I believe works better for our capacity.

Thanks for the PR!