<div dir="ltr">Hi DM<div><br></div><div style>On the following:</div><div><span style="font-family:arial,sans-serif;font-size:13px"><b>I'm wondering whether it'd be cheaper to build a list of those that aren't present. (Just wondering).</b></span><br>
</div><div style><span style="font-family:arial,sans-serif;font-size:13px">CJB>> Perhaps, but we'll still need to set each bit in the bitset. And for Old Testaments that only have the Psalms this may be slower. But are you pointing towards having an AntiBitSetPassage which would store the results in reverse? I've got this running fast enough for me, i.e. 3ish seconds to get all global lists from all modules. (generates about 13MBs of sitemap, so my lost time is now spent in network transfer). So I'm happy enough.</span></div>
<div style><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><div style="font-family:arial,sans-serif;font-size:13px"><b>Also have been musing on whether it'd be good to modify Bitwise passage to hold two testament bit maps. And null if not needed. Or maybe one per book.<br>
</b></div><div style="font-family:arial,sans-serif;font-size:13px">CJB>>Not entirely sure how this would help in this case, because presumably the BitSet is a O(1) operation for setting. I guess it speeds up for contains() in that you may not need to look at a number of bits. Before we go down that root, it would make sense to look at exactly where time is being spent on an Android device. I'm thinking we're better spending time on optimizing those areas (which are likely to be XML related) rather than the other ones. </div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">It might be good to add a method to SwordUtils with the same decode params and determines whether the array has content. (i.e. is zero). This is in a tight loop, so would be fast.</div>
</div><div style="font-family:arial,sans-serif;font-size:13px">CJB>> Not quite sure what you're getting at here. I did add a comment, which effectively said that:</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div><div><font face="arial, sans-serif"> // can this be simplified to temp[8] == 0 && temp[9] == 0?</font></div><div><font face="arial, sans-serif"> int verseSize = SwordUtil.decodeLittleEndian16(temp, ii + 8);</font></div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div></div><div style="font-family:arial,sans-serif;font-size:13px">so presumably instead of calling out to decodeLittleEndian16(), we could simply have the expression in the comment? But again, small performance (< 1% I reckon) improvements compared to the overall operations.</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">I've committed an extra couple of bits as per your original comments.</div><div style="font-family:arial,sans-serif;font-size:13px">
Chris</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 15 February 2013 22:21, Chris Burrell <span dir="ltr"><<a href="mailto:chris@burrell.me.uk" target="_blank">chris@burrell.me.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Let me push my changes: <a href="https://github.com/tyndale/jsword/commit/c0d50633b28348185035a59bc9738672a0db5b42" target="_blank">https://github.com/tyndale/jsword/commit/c0d50633b28348185035a59bc9738672a0db5b42</a> <div>
<br></div><div>(There's the missing data file stuff in there too - let me know if you want it separated out).</div><div><br></div><div>The reason for the name change is that there was no method initially. it just used contains() on the backend. The Book interface has not changed, but tries a call to backend.getFGKL() and if the method is unsupported it reverts back to the existing functionality. The name change was to highlight that SwordBook has a good way of getting the FGKL</div>
<div><br></div><div><br></div><div>I did this before I realised we only had 1 implementation of a AbstractPassageBook. And I moved the default from AbstractPassageBook because it required access to backend, so I had a choice of exposing the backend or moving the function. I'm not thinking entirely straight and it may be have been better to expose backend instead.</div>
<div><br></div><div>I take your comments in both emails and I'll look at implementing some of them tomorrow.</div><div><br></div><div><div> /* (non-Javadoc)</div><div> * @see org.crosswire.jsword.book.Book#getGlobalKeyList()</div>
<div> */</div><div class="im"><div> public final Key getGlobalKeyList() {</div><div> if (global == null) {</div></div><div> try {</div><div> global = this.backend.getFastGlobalKeyList();</div>
<div> return global;</div>
<div> } catch (UnsupportedOperationException ex) {</div><div> // fail silently, operation not supported by the backend</div><div> log.debug(ex.getMessage());</div><div> } catch (BookException ex) {</div>
<div> // failing silently, as previous behaviour was to attempt to</div><div> // return as much as we can using the slower method</div><div> log.debug(ex.getMessage());</div><div>
}</div><div><br></div><div> Versification v11n = super.getVersification();</div><div><br></div><div> global = super.createEmptyKeyList();</div><div> Key all = PassageKeyFactory.instance().getGlobalKeyList(v11n);</div>
<div class="im">
<div><br></div><div> for (Key key : all) {</div><div> if (contains(key)) {</div><div> global.addAll(key);</div><div> }</div><div> }</div><div> }</div>
<div><br></div><div> return global;</div><div> }</div><div><br></div></div></div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Chris</div></font></span></div><div class="HOEnZb"><div class="h5">
<div class="gmail_extra"><br><br><div class="gmail_quote">On 15 February 2013 22:08, DM Smith <span dir="ltr"><<a href="mailto:dmsmith@crosswire.org" target="_blank">dmsmith@crosswire.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Contains(xxx) is correct. So don't need to concern about the indirection.<div><br>
</div><div>There are three parts to a compressed testament. The one index points to a second index which points to the data.</div><span><font color="#888888"><div>-- DM</div></font></span><div><div>
<div><br><div><div>On Feb 15, 2013, at 5:05 PM, Chris Burrell <<a href="mailto:chris@burrell.me.uk" target="_blank">chris@burrell.me.uk</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">I'm confused about the double indirection in that the contains() method doesn't use it which is what I based my bit on.<div>
<br></div><div>It simply checked for the size given an ordinal. So I'm doing the opposite, getting all the data first, then iterating through them looking for non-zero sizes</div>
<div><br></div><div>I'm pretty sure it works (tested a little bit). It's shaved off heaps off time (several minutes to a few seconds for all 250 modules I have installed), but obviously that's no good if it isn't doing the right thing!</div>
<div>Chris</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 15 February 2013 21:59, DM Smith <span dir="ltr"><<a href="mailto:dmsmith@crosswire.org" target="_blank">dmsmith@crosswire.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I like the direction this is heading. I'm not sure if it properly handles the double indirection of the compressed module. (I didn't look.) So assuming it is....</div>
<div><br></div><div>Just directly create and use a bitwise passage. don't get it from the factory.</div><div><br></div><div>The purpose of the factory was to be able to switch between implementations.</div><div><br></div>
<div>I added the two bitwise passage classes and it is pretty much all we use now.</div><div><br></div><div>I'm wondering whether it'd be cheaper to build a list of those that aren't present. (Just wondering).</div>
<div><br></div><div>Also have been musing on whether it'd be good to modify Bitwise passage to hold two testament bit maps. And null if not needed. Or maybe one per book.</div><div><br></div><div>Anyway, by writing to an api we can swap out the implementation. (We should add the new method as an overload by name to Passage and abstract passage. That way you don't need to know what kind of passage it is.)</div>
<div><br></div><div>It might be good to add a method to SwordUtils with the same decode params and determines whether the array has content. (i.e. is zero). This is in a tight loop, so would be fast.</div><div><div>
<div><br></div><div><br></div><div><div><div>On Feb 15, 2013, at 3:26 PM, Chris Burrell <<a href="mailto:chris@burrell.me.uk" target="_blank">chris@burrell.me.uk</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr">
Hi DM<div><br></div><div>I'm about to test the following method for at least all Z backends:</div><div><br></div><div>Maybe you can tell me what you think of it:</div><div><br></div>
<div><b>getFastGlobalKeyList</b><br></div><div><br></div><div><div>public Key getFastGlobalKeyList() throws BookException {</div><div> ZVerseBackendState rafBook = null;</div><div> try {</div>
<div> rafBook = initState();</div><div><br></div><div> String v11nName = getBookMetaData().getProperty(ConfigEntryType.VERSIFICATION).toString();</div><div> Versification v11n = Versifications.instance().getVersification(v11nName);</div>
<div><br></div><div> Testament[] testaments = new Testament[] {</div><div> Testament.OLD, Testament.NEW</div><div> };</div><div> </div><div> Key globalList = PassageKeyFactory.instance().createEmptyKeyList(v11n);</div>
<div> Passage passage = KeyUtil.getPassage(globalList, v11n);</div><div> BitwisePassage bitwisePassage = null;</div><div> if(passage instanceof BitwisePassage) {</div><div> bitwisePassage = (BitwisePassage) passage;</div>
<div> bitwisePassage.raiseEventSuppresion();</div><div> bitwisePassage.raiseNormalizeProtection();</div><div> }</div><div> </div><div> </div><div> for (Testament currentTestament : testaments) {</div>
<div> RandomAccessFile compRaf = currentTestament == Testament.NEW ? rafBook.getNtCompRaf() : rafBook.getOtCompRaf();</div><div><br></div><div> // If Bible does not contain the desired testament, then false</div>
<div> if (compRaf == null) {</div><div> // no keys in this testament</div><div> continue;</div><div> }</div><div><br></div><div> int maxIndex = v11n.getCount(currentTestament);</div>
<div><br></div><div> // Read in the whole index, a few hundred Kb at most.</div><div> byte[] temp = SwordUtil.readRAF(compRaf, 0, COMP_ENTRY_SIZE * maxIndex);</div><div><br></div><div> // for each block of 10 bytes, we consider the last 2 bytes.</div>
<div> for (int ii = 0; ii < temp.length; ii += 10) {</div><div> // can this be simplified to temp[8] == 0 && temp[9] == 0?</div><div> int verseSize = SwordUtil.decodeLittleEndian16(temp, 8);</div>
<div> </div><div> //can this be optimized even further - i.e. why decodeOrdinal, when add() go simply pass in and store an ordinal</div><div> if (verseSize > 0) {</div>
<div> if(bitwisePassage != null) {</div><div> bitwisePassage.addVersifiedOrdinal(ii % 10);</div><div> } else {</div><div> globalList.addAll(v11n.decodeOrdinal(ii % 10));</div>
<div> }</div><div> }</div><div> }</div><div> }</div><div> </div><div> if(bitwisePassage != null) {</div><div> bitwisePassage.lowerNormalizeProtection();</div>
<div> bitwisePassage.lowerEventSuppressionAndTest();</div><div> }</div><div><br></div><div> return globalList;</div><div> } catch (IOException e) {</div><div> throw new BookException(JSMsg.gettext("Unable to read key list from book."));</div>
<div> } finally {</div><div> IOUtil.close(rafBook);</div><div> }</div><div> }</div><div><br></div><div><br></div><div><b>addVersifiedOrdinal (is just a simplification of add, so that we can give the ordinals directly, rather than converting to a verse and back again).</b><br>
</div><div><div>/**</div><div> * A shortcut to adding a key, by ordinal. The ordinal needs to be taken from the same versification as the passage being created.</div><div> *</div><div> * @param ordinal the ordinal</div>
<div> */</div><div> public void addVersifiedOrdinal(int ordinal) {</div><div> Versification v11n = getVersification();</div><div> optimizeWrites();</div><div><br></div><div> store.set(ordinal);</div>
<div><br></div><div> // we do an extra check here because the cost of calculating the</div><div> // params is non-zero and may be wasted</div><div> if (suppressEvents == 0) {</div><div> Verse verse = v11n.decodeOrdinal(ordinal);</div>
<div> fireIntervalAdded(this, verse, verse);</div><div> }</div><div> }</div></div></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 15 February 2013 20:22, DM Smith <span dir="ltr"><<a href="mailto:dmsmith@crosswire.org" target="_blank">dmsmith@crosswire.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">That's what <a href="http://www.crosswire.org/tracker/browse/JS-246" target="_blank">http://www.crosswire.org/tracker/browse/JS-246</a> is all about.<br>
<br>
There are a good number of places that we get all the keys for a module. But the construction of the list is dumb.<br>
<br>
The question is what do you want? Do you want all the possible keys in a versification? Or do you want the keys for the verses that are actually in a module?<br>
<br>
If you only want chapters that exist, consider working off of Versification to get the book names and the chapters. For each chapter, get the number of verses and grab one (maybe verse 1 or one halfway into the chapter) on the assumption that if one exists then the rest do.<br>
<br>
<br>
Calling book.contains(key) is what you want to do. It goes against the backend to determine whether the verse is in the module.<br>
<br>
Don't call book.getGlobalKeyList(); We probably should deprecate the method. It is too easy to call it and it is very expensive for what it does.<br>
<br>
I have a 3 day weekend and hope to finish the av11n work. It impacts this. Will probably add iterators for a versification.<br>
<br>
In Him,<br>
DM<br>
<div><br>
<br>
<br>
On Feb 15, 2013, at 2:14 PM, Chris Burrell <<a href="mailto:chris@burrell.me.uk" target="_blank">chris@burrell.me.uk</a>> wrote:<br>
<br>
> Hi all<br>
><br>
> getGlobalKeyList() seems to do a lot of work against the file system.<br>
><br>
> I'm attempting to create a sitemap, which includes a URL to every chapter. Presumably getGlobalKeyList is what I want to ensure that I'm only displaying those chapters that exist. I was intending to do:<br>
><br>
> globalKeyList = getGlobalKeyList();<br>
> for(each book in versification) {<br>
> myBookKey = book.getValidKey(book.getShortName())<br>
> myBookKey.retainAll(globalKeyList);<br>
><br>
> if(myBookKey.getCardinality() > 0) {<br>
> outputSiteMapChapter();<br>
> }<br>
> }<br>
><br>
> So for each version I call getGlobalKeyList(), but that in terms seem to make individual reads (through the contains()) method for every potential key in the versification.<br>
><br>
> public final Key getGlobalKeyList() {<br>
> if (global == null) {<br>
> Versification v11n = Versifications.instance().getVersification(versification);<br>
> global = keyf.createEmptyKeyList(v11n);<br>
> Key all = keyf.getGlobalKeyList(v11n);<br>
> for (Key key : all) {<br>
> if (contains(key)) {<br>
> global.addAll(key);<br>
> }<br>
> }<br>
> }<br>
> return global;<br>
> }<br>
><br>
> The contains() method above makes a backend call every time.<br>
><br>
> Isn't there a way to obtain the list of keys from the index? I'm not quite sure where the versification comes in here, or is it there simply to map the names of the keys to their positions in the index file? If so, can we simply read all keys in OT and NT, and check<br>
><br>
> Doing this on GlobalKeyList() would also speed up index creation.<br>
><br>
> Finally, it seems contains() only works on a single key, or takes the first key/verse if it's a passage. Presumably that is correct?<br>
><br>
> Chris<br>
><br>
</div>> _______________________________________________<br>
> jsword-devel mailing list<br>
> <a href="mailto:jsword-devel@crosswire.org" target="_blank">jsword-devel@crosswire.org</a><br>
> <a href="http://www.crosswire.org/mailman/listinfo/jsword-devel" target="_blank">http://www.crosswire.org/mailman/listinfo/jsword-devel</a><br>
<br>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div></div></blockquote></div><br></div>
</blockquote></div><br></div></div></div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>