[jsword-devel] Activator classes across the JSword library + Refactoring for JS-109

[Chris Burrell]

For those who want the details of the analysis so far:

We start with AbstractPassageBook.getOsisIterator. This core function
essentially obtains the XML and converts it to a XML structure.

   - This iterates over ranges first
      - within each range, (1)
         - adds some headers to the xml document (2)
         - iterates over each verse (3)
            -  calls getRawText
               - getRawText in turn calls the relevant backend
               - The backend, calls activate
                  - uses the stored fields to retrieve the random access
                  files that have already been opened (and cause the problem)
                  - returns OSIS in raw "string" form (4)
               - adds the xml to the xml document (5)

My suggested refactoring, would be to re-jig things as follows:

   - (removed)
      - (removed)
         - (removed)
         - (removed)
            -  calls getRawText
               - getRawText in turn calls the relevant backend
               - The backend, calls activate (? no longer required?)
                  - uses the stored fields to retrieve the random access
                  files that have already been opened (and cause the problem)
                  - *within each range, (1)*
                  - *use passed-in parameter and adds some headers to the
                  xml document (2)*
                  - *iterates over each verse (3)*
                     - returns OSIS in raw "string" form (4)
                     - *adds the xml to the xml document (5)*


Something along those lines anyway (lines in bold have moved/changed
slightly)
Chris



On 23 August 2012 22:33, Chris Burrell <chris at burrell.me.uk> wrote:

> Hi all
>
> Just wondering if this section of the "Activator" code was introduced to
> reduce memory as a result of observing an issue, or not? I'm looking at
> SwordGenBook in particular, but seeing all kinds of interesting things
>
>
>    - Activator has a reduceMemoryUsage method. This is based on KILL. Of
>    the 3 enum constants, only 1 has a method that actually does something. And
>    as far as I can tell, anyone can call reduceMemoryUsage at any time, which
>    will then thoroughly break anything in the process of running by nulling
>    out a whole load of references.
>    - Secondly, there is this Lock object. It's not used to synchronise
>    on, nor used in any form. The private javadoc comment is slightly
>    mis-leading, unless I'm missing a piece of code somewhere.
>    - SwordGenBook.activate gets an empty set from its parent, and thereby
>    the loop doesn't do anything.
>    - the "global" field is always an empty ReadOnlyList as a result, can
>    we remove and simplify this?
>
> In the SwordBook:
>
>    - activate() does nothing (apart from registering it with the
>    activator, which seems to defeat the point as there is no clean-up)
>    - deactivate() deactivates the backend it is based upon. Should this
>    be done preferably by the backend rather than here?
>
> Finally, in order to address JS-109 and contain the refactoring process, I
> am thinking of changing the method signature for
> AbstractPassageBook.getRawText(). My intended approach is to pass in an
> extract parameter, as 1 or 2 anonymous class to post-process what has been
> discovered in reading the raw data. By doing so, I believe I might be able
> to push both loops from AbstractPassageBook into the backends, so that the
> backends themselves deal with iterating through ranges and verses.
>
> The idea being, for performance reasons, we want to keep the random access
> files open and re-use the state. However, we don't want to be sharing the
> state across threads, and therefore that state needs to be local. Not sure
> if it's going to be feasible doing it that way, but I thought I might give
> it a go. It obviously might mean re-jigging the vast majority of the
> backends. Shout if you want some more details.
>
> Cheers
> Chris
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.crosswire.org/pipermail/jsword-devel/attachments/20120823/0ce2d606/attachment.html>