On Fri, Jun 08, 2012 at 02:24:32PM +0000, [email protected] wrote: > http://defect.opensolaris.org/bz/show_bug.cgi?id=19215 > > --- Comment #13 from Gustavo Lopes <[email protected]> 2012-06-08 > 14:24:28 UTC --- > Make that > > https://bitbucket.org/cataphract/opengrok/compare/bug19215..ce0b759 > > now that I've updated the default branch.
Hmmm, strange, the PageConfig.java diff of that URL doesn't show up on my desktops (probably because of a 403 error) ... Anyway, I cloned this repo and made a webrev against 1385:1387 to have something referrable: see http://iws.cs.uni-magdeburg.de/~elkner/tmp/gustavo/ But first: The main problem wrt. leaks is, that the Searchers do not auto-close the used IndexReaders/Executors anymore. So to restore the old behavior (fix the bug), something like this should be sufficient: http://src.iws.cs.ovgu.de/source/diff/opengrok-jel/src/org/opensolaris/opengrok/web/SearchHelper.java?r1=/opengrok-jel/src/org/opensolaris/opengrok/web/SearchHelper.java@1385:1c62bc7908a0&r2=/opengrok-jel/src/org/opensolaris/opengrok/web/SearchHelper.java@1424:504c5781fb6a&format=u&full=0 Wrt. to the webrev/Gustvos patches: 1) src/org/opensolaris/opengrok/web/WebappListener.java: a) Shouldn't runtime related settings/instanca be fetched via the RuntimeEnvironment, e.g. like RuntimeEnvironment.getInstance()[.getConfiguration()].getSearchPool[Size]() Furthermore, even if obtained via RuntimeEnvironment, shouldn't lazy instantiation should be preferred? IMHO: no changes required here 2) web/search.jsp a) The SearchHelper should not have have any dependencies on a webapp/ servlet context/its consumer - it should be usable in a "standalone" app in the same manner. As shown above, there is no need for such a reference. So IMHO no changes required here as well. 3) src/org/opensolaris/opengrok/web/PageConfig.java a) same thing - no changes required at all. 4) src/org/opensolaris/opengrok/configuration/RuntimeEnvironment.java a) line 41: import not needed - no changes required 5) src/org/opensolaris/opengrok/configuration/Configuration.java: - line 195: a little nit - not required, since globals are always initialized to 0/null 6) src/org/opensolaris/opengrok/web/SearchHelper.java Changes look IMHO a little bit chaotic: a) "this." prefix == overhead - shouldn't be used if not required (readability, unwritten project code convention) b) Lines 103-108: Why public? IMHO there is no need/is dangerous to expose these impementation details ... c) Lines 107,108: Why is this one needed? If programmed cleanly, doesn't this list contain one entry, only? the IndexSearcher which is equal to this.searcher - i.e. we already have a reference to it ? d) Lines 103..105: Why is this needed here? SearcherManagers are acquired from the "SearcherCache" based on the index dir. Why is it not possible to aquire the needed Searcher directly from the "SearcherCache" based on the index dir? So IMHO, if such a list is needed, it should be part of the SearchManager. Isn't it pretty unclean/dangerous, to let an "outsider" manage/control the innards of the SearcherCache? IMHO better encapsulation is needed. e) Lines 133..143: not needed - see 1) f) Lines 184..227: as said in d) - any reference/occurance of SearcherManager looks like a design flaw - should go into "SearcherCache" g) Lines 210..222: tribut to unclean design? Asking the SearcherCache for Searcher for certain dirs just to get a reference to its internally used IndexReaders to build a new Searcher from it - does this make any sense? Sounds for me from the back through the breast and also rises the question: Do we have a SearcherCache or not? Why does one need to bypass it from time to time? h) Line 227: similar to g) - Why does the SearchHelper need to know anything about an executor, when it doesn't need it - the obvious owner/user/manager seem to be the SearcherCache? i) Lines 460-472: as said, seems to be odd as well, since a SearchHelper has one aka this.searcher, only ... 7) src/org/opensolaris/opengrok/search/SearcherCache.java a) big flaw: there is no way to shutdown this Cache. So a restart of the web app may cause big leaks as well ... Also resources can't be freed, even if there are no consumers for a long time ... b) Lines 70-72: the internally used threadPool, which is essential for proper working, should not be exposed to outsiders. c) Lines 48-54: The current behavior is, that each request gets a new ThreadPool with max 2*(CPUs+1), if required. Obviously not that smart. However, with the suggested change there are max. only 2*(CPUs+1) threads available for all requests. Depending on the load (number of concurrent requests and queried projects), this may actually result in blocking requests (kind of sequential behavior) and lead to timeouts wrt. answers. Not sure, what the search threads are actually doing, but may be a FixedThreadPool may cause more problems, than it solves. Unless no deeper research/experience was gathered, for now I would probably prefer a dynamic Thread Pool Executor with a core pool size of 2*(CPUs+1) and a max. pool size of e.g. 128 or 256 threads (depending on the number of projects, machine, ...). JavaConsole/JMX might be used to find out more ... d) Lines 58..63: synchronizing newThread just because of getting a unique int is a bad choice. It should be unsynced and use an AtomicInteger instead. e) Line 64: a tribute to the missing shutdown method? ;-) f) Line 77..84: 1) Why is it necessary to create a new SearcherFactory for each index dir? Shouldn't factories be singletons a priori? 2) Does it make sense, that a http thread "forks" another thread and blocks until it has finished its work? I.e. the search within a single dir (project) should always be done by the [http] thread itself, w/o the use of another thread ... 3) Since the whole thing is to avoid unnecessary Searcher/Reader creation, shouldn't be a more clever algorithm be used to obtain SearchManagers/maintain the map? I think it would be better to use a normal map and a ReentrantLock -> lookup again -> on fail create and add instead of blindly creating and eventuelly discarding a SM+Searcher+Reader ... g) Last but not least, what happens, if the configuration (data root) changes? A better way to implement? a) SearcherCache: could be either a singleton - SearcherCache.getInstance() - or a "normal" Instance managed by the RuntimeEnvironment (RE) - the easy way wrt. management, e.g.: - RE.getSearcherCache() creates lazy a instance of the SearcherCache if not already done, keeps a ref to it and returns it - on RE.setConfiguration() [data root change] it may shutdown the SC and set the internal ref to null - there should be only one instance of a SearcherFactory be used by the SC - the SearcherFactory should create newSearcher with a threadpool arg only, if it has subreaders - the sm.maybeRefresh() part can be ommitted, if the cache gets shutdowned/a new one is used, when the RE.config gets changed - implementation details like threadpool, SM usage should be kept private - misc bla mentioned above b) - SearchHelper should get a ref to the SC either via SearcherCache.getInstance() or RE.getSearcherCache() - the only thing it does, is sc.getSearcher(File ....) and - on destroy: sc.release(searcher) NOTE: The implementation details, whether the SC only caches readers and creates each time a new Searcher on getSearcher(...) or caches created searchers, is completely hidden from the consumer (e.g. SearchHelper) and thus can be changed/tuned without any trouble if necessary ... Hopefully I've catched most issues/problems ;-) Cheers, jel. -- Otto-von-Guericke University http://www.cs.uni-magdeburg.de/ Department of Computer Science Geb. 29 R 027, Universitaetsplatz 2 39106 Magdeburg, Germany Tel: +49 391 67 12768 _______________________________________________ opengrok-dev mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/opengrok-dev
