#38 Reduce locking on the class cache

None
closed-fixed
nobody
None
5
2014-10-12
2012-04-04
Johno Crawford
No

Same goal as https://github.com/freemarker/freemarker/pull/2 however it would be nice if this were part of 2.3.x. Please find attached patch.

Discussion

  • It imports imports java.util.concurrent.ConcurrentHashMap... that's there only since J2SE 1.5. 2.3.x only requires 1.2.

     
  • I have just attached your patch (sent to me in e-mail) that should be backward compatible...

     
  • I believe it's not safe to call those methods outside synchronization, because the new items are added to the map concurrently. Or is there something in the API that says otherwise?

    But anyway, decreasing locking on the class cache is hight priority TODO, so whenever 2..3.20 is released it will be almost surely part of it. (The problem that will addressed there is that the map remains locked during introspection. After all, doing a Map.get inside sync. block is not a big deal, until the instrospection starts blocking gets. Using ConcurrentHashMap on Java 1.5+ was also considered, but I'm nor sure what was the result... maybe that it's overhead killed its advantage if there's was no long locking anyway.)

     
  • This is now fixed in the 2.3.x head. However, 2.3.20 is only expected on the end of May. But until that, I think it's already safe to use.

     
  • To be clear, actually I haven't used this patch, as I did other refactorings too. But it uses ConcurrentHashMap when available, so it's similar in that respect.

     
  • Andy Tran
    Andy Tran
    2013-07-31

    Hey Daniel, thanks for fixing this concurrency issue in 2.3.20.

    I was wondering if it was a conscious decision to not include this in freemarker.ext.beans.OverloadedMethodsSubset.selectorCache? I found this to be a bottle neck on one of our pages and fixed it in our fork which just uses a ConcurrentHashMap.

    I ask this because I see a TODO

    // TODO: make it not concurrent
    private final Map selectorCache = new HashMap();

     
    Last edit: Andy Tran 2013-07-31
  • I thought that's not very important as each method name has different lock, and so I haven't yet fixed that... But sure, I should. I add this to the 2.3.21 TODO.

     
  • This is now fixed on GitHub.

     
    • status: open --> closed-fixed
    • Group: -->
     
  • Fixed in 2.3.21