Details

      Description

      Every time a commit happens two Stats instances [org.apache.solr.common.util.ConcurrentLRUCache.Stats] are leaking.

      Following code [org.apache.solr.search.FastLRUCache] to maintain cumulative cache statistics causing this Stats object leak.

          cumulativeStats = (List<ConcurrentLRUCache.Stats>) persistence;
          cumulativeStats.add(cache.getStats());
      

      Everytime a commit happens a new cache object is getting created and its stats is added to the list which is not released at all.

      1. SOLR-1798.patch
        0.4 kB
        Yonik Seeley
      2. SOLR-1798.patch
        4 kB
        Yonik Seeley

        Activity

        Hide
        Yonik Seeley added a comment -

        Thanks Laxman, that does look incorrect.

        Noble - do you remember why a List was used here?

        Show
        Yonik Seeley added a comment - Thanks Laxman, that does look incorrect. Noble - do you remember why a List was used here?
        Hide
        Laxman added a comment -

        Thanks for quick reponse Yonik.

        Following is our analysis so far which may help us in resolving the issue.

        1) For every commit cache instances are getting created.[org.apache.solr.search.SolrIndexSearcher]
        2) For each cache config we are maintaining a list of Statistics for aggregation.

        One more observation
        From the Solr stats page I see all the cache stats as 0 values. Following line of code from FastLRUCache.init looks incorrect.

            cache.setAlive(false);
        

        Because of the above line, all the cache instances created from FastLRUCache are not maintaining any Stats info.
        If this is correct, I dont understand why are we maintaining a list of stats containing zero values.

        Or did I miss something important here?

        Show
        Laxman added a comment - Thanks for quick reponse Yonik. Following is our analysis so far which may help us in resolving the issue. 1) For every commit cache instances are getting created. [org.apache.solr.search.SolrIndexSearcher] 2) For each cache config we are maintaining a list of Statistics for aggregation. One more observation From the Solr stats page I see all the cache stats as 0 values. Following line of code from FastLRUCache.init looks incorrect. cache.setAlive(false); Because of the above line, all the cache instances created from FastLRUCache are not maintaining any Stats info. If this is correct, I dont understand why are we maintaining a list of stats containing zero values. Or did I miss something important here?
        Hide
        Yonik Seeley added a comment -

        I think the fix is as simple as this patch.
        Verifying that it fixes it is slightly harder - I'll try breaking out the profiler.

        Show
        Yonik Seeley added a comment - I think the fix is as simple as this patch. Verifying that it fixes it is slightly harder - I'll try breaking out the profiler.
        Hide
        Yonik Seeley added a comment -

        Hmmm, right - fixes the memory leak, but breaks cumulative stats.

        Show
        Yonik Seeley added a comment - Hmmm, right - fixes the memory leak, but breaks cumulative stats.
        Hide
        Yonik Seeley added a comment -

        New patch that fixes the cumulative stats by adding in an entry in the list for cumulative stats and incrementing them for every cache that is closed.

        Longer term, we might want a hash set or something (if we start having per-segment caches) but more changes will be necessary then anyway.

        Show
        Yonik Seeley added a comment - New patch that fixes the cumulative stats by adding in an entry in the list for cumulative stats and incrementing them for every cache that is closed. Longer term, we might want a hash set or something (if we start having per-segment caches) but more changes will be necessary then anyway.
        Hide
        Yonik Seeley added a comment -

        committed.

        Show
        Yonik Seeley added a comment - committed.
        Hide
        Noble Paul added a comment -

        do you remember why a List was used here?

        It was used to collect the cumulative data. I guess the original LRU cache was doing it the same way. It was a wrong decision to make

        Show
        Noble Paul added a comment - do you remember why a List was used here? It was used to collect the cumulative data. I guess the original LRU cache was doing it the same way. It was a wrong decision to make
        Hide
        Hoss Man added a comment -

        Correcting Fix Version based on CHANGES.txt, see this thread for more details...

        http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

        Show
        Hoss Man added a comment - Correcting Fix Version based on CHANGES.txt, see this thread for more details... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E
        Hide
        Hoss Man added a comment -

        Committed revision 949476.

        merging to branch-1.4 for 1.4.1

        Show
        Hoss Man added a comment - Committed revision 949476. merging to branch-1.4 for 1.4.1

          People

          • Assignee:
            Yonik Seeley
            Reporter:
            Laxman
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development