Thanks Julian,<div>I just checked in 12408 with this change. I couldn't see a way to use validator to check type in this case, since result.get(0) will return a Member[] or a Member. If you can give me some hints about how I might use validator I can give it another pass.</div>
<div><br></div><div>-matt</div><div><br></div><div><br><div class="gmail_quote">On Thu, Mar 5, 2009 at 2:20 PM, Julian Hyde <span dir="ltr"><<a href="mailto:jhyde@pentaho.com">jhyde@pentaho.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">As usual, my reaction to this code is
'yuck'.</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">I think your change is sound, and you should go
ahead.</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">A few other improvements to consider:</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">1. To distinguish between a tuple-set and a member-set, see
if you can use the type derived by the validator rather than the runtime
'instanceof' check.</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">2. The code seems to be detecting empty lists by doing a
get(0) then handling the IndexOutOfBoundsException. I believe that the person
who wrote the high-cardinalty dimension code did this because size() can be an
expensive operation; but they may think that try-catch is more elegant than if,
I don't know. It's not possible to make size() a cheap operation, but we can
make isEmpty() efficient. Can you see whether it's possible to replace it with
an isEmpty check?</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">3. Even though I don't like the high-cardinality dimension
code as a whole, ConcatenableList is basically sound, and worth saving. The
change you proposed earlier, to consolidate a the sublists of ConcatenableList
into a single array on the first call to size or get, damages the performance
characteristics of that class. For instance, it is not worth consolidating a
list that has two sublists of size 500K, but it is worth consolidating a list
that has 100K lists of size 10. In particular, your approach would give O(n^2)
performance if someone adds one element at a time onto the end of a
list.</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">This looks like a better algorithm: consolidate only if the
number of sublists is greater than the square root of the number of
elements.</font></span></div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2"></font></span> </div>
<div dir="ltr" align="left"><span><font face="Lucida Sans" color="#000080" size="2">Julian</font></span></div><br>
<blockquote style="padding-left:5px;margin-left:5px;border-left:#000080 2px solid;margin-right:0px">
<div lang="en-us" dir="ltr" align="left">
<hr>
<font face="Tahoma" size="2"><div class="im"><b>From:</b> <a href="mailto:mondrian-bounces@pentaho.org" target="_blank">mondrian-bounces@pentaho.org</a>
[mailto:<a href="mailto:mondrian-bounces@pentaho.org" target="_blank">mondrian-bounces@pentaho.org</a>] <b>On Behalf Of </b>Matt
Campbell<br></div><b>Sent:</b> Thursday, March 05, 2009 10:30 AM<div class="im"><br><b>To:</b>
Mondrian developer mailing list<br></div><b>Subject:</b> Re: [Mondrian] Re:
Performance degradation<br></font><br></div><div><div></div><div class="h5">
<div></div>
<div><br></div>
<div>After spending some more time looking at this issue I think the best
solution (at least short term), is to fix the method SetFunDef.evaluateList
(pasted below). I believe what it's intended to do is load an ArrayList
if the dimension in the set is not flagged as highCardinality. It fails
in certain cases, though, since the result object may be a list of lists when
there are multi-element tuples.</div>
<div><br></div>
<div>I think a reasonable modification would be to assume the set is NOT
highCardinality, unless there is only a single dimension in result. This would
cause SetFunDef to use an ArrayList in such cases, which was the behavior
prior to the high cardinality changes.</div>
<div><br></div>
<div>Any objections to such a change? </div>
<div><br></div>
<div>(note that the method below is as it is currently checked in)</div>
<div><br></div>
<div>
<div>public List evaluateList(final Evaluator evaluator) {</div>
<div> this.result = new
ConcatenableList();</div>
<div> for (VoidCalc voidCalc :
voidCalcs) {</div>
<div>
voidCalc.evaluateVoid(evaluator);</div>
<div> }</div>
<div> // REVIEW: What the heck
is this code doing? Why is it OK to</div>
<div> // ignore
IndexOutOfBoundsException and NullPointerException?</div>
<div> try {</div>
<div> if
(result.get(0) instanceof Member) {</div>
<div>
if (!((Member)result.get(0)).getDimension()</div>
<div>
.isHighCardinality()) {</div>
<div>
result.toArray();</div>
<div>
}</div>
<div> }</div>
<div> } catch
(IndexOutOfBoundsException iooe) {</div>
<div> } catch
(NullPointerException npe) {</div>
<div> }</div>
<div> return result;</div>
<div> }</div></div><br><br>
<div class="gmail_quote">On Wed, Mar 4, 2009 at 4:16 PM, Matt Campbell <span dir="ltr"><<a href="mailto:mkambol@gmail.com" target="_blank">mkambol@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="padding-left:1ex;margin:0px 0px 0px 0.8ex;border-left:#ccc 1px solid">Virtually
all the time spent on the query is spent in ConcatenableList, in the size()
and get() methods. Both have some pretty inefficient looking logic.
In get(), if the element retrieved is not the next, previous, or
pre-previous, then the code iterates through the whole list up to the
elements index. Similarly with .size(), the code iterates through the
lists, adding up each lists size. I'm guessing that in many cases the
expense of doing this once is probably about the cost of converting to an
ArrayList. As an experiment I modified .size() to always convert to an
ArrayList, and this cut the time to execute dramatically.
<div>
<div></div>
<div>
<div><br></div>
<div>
<div>
<div>
<div><br>
<div class="gmail_quote">On Wed, Mar 4, 2009 at 12:02 PM, Matt Campbell <span dir="ltr"><<a href="mailto:mkambol@gmail.com" target="_blank">mkambol@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="padding-left:1ex;margin:0px 0px 0px 0.8ex;border-left:#ccc 1px solid">After
syncing to a changelist I was still seeing lots of files in my directories
that were showing version 0 in perforce. I eventually got past this
by starting from a totally clean slate and syncing.
<div><br></div>
<div>Still investigating, but it looks like changelist 11049 is the
culprit. The query I posted runs in 2.5 seconds with change 11048,
and takes a full minute and a half with 11049.
<div>
<div></div>
<div><br><br>
<div class="gmail_quote">On Wed, Mar 4, 2009 at 11:16 AM, Julian Hyde <span dir="ltr"><<a href="mailto:jhyde@pentaho.com" target="_blank">jhyde@pentaho.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="padding-left:1ex;margin:0px 0px 0px 0.8ex;border-left:#ccc 1px solid">
<div bgcolor="#FFFFFF">
<div>There is no version 0 as such. If p4 sync 'gets' version 0, it
removes the file from your sandbox. Is that what you are seeing?
<div>
<div></div>
<div><br><br>On Mar 4, 2009, at 6:36 AM, Matt Campbell <<a href="mailto:mkambol@gmail.com" target="_blank">mkambol@gmail.com</a>>
wrote:<br><br></div></div></div>
<div>
<div></div>
<div>
<div></div>
<blockquote type="cite">
<div>Hmmm. I see many files in my directories that are marked as
having version 0. For example, I see MemberIterCalc.java in my
sandbox, even though it was added much after the changelist I synched
to. Any ideas how this could happen?<br><br>
<div class="gmail_quote">On Wed, Mar 4, 2009 at 9:13 AM, John V. Sichi
<span dir="ltr"><<a href="mailto:jsichi@gmail.com" target="_blank"></a><a href="mailto:jsichi@gmail.com" target="_blank">jsichi@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="padding-left:1ex;margin:0px 0px 0px 0.8ex;border-left:#ccc 1px solid">
<div>Matt Campbell wrote:<br>
<blockquote class="gmail_quote" style="padding-left:1ex;margin:0px 0px 0px 0.8ex;border-left:#ccc 1px solid">I've
been trying to isolate when this issue was introduced, but am
struggling a bit with perforce. I'm trying to sync to a
changelist using something like:<br><br>p4 sync
//open/mondrian/...@10480<br><br>This brings in the set of files
from that changelist, but does not appear to delete files from
future changelists. <br>What is the correct way to sync to a
changelist?<br></blockquote><br></div>That is the correct command.
It will delete files from future changelists, but it will not
delete empty directories unless you have the rmdir option set in
your client (the default is normdir).<br><br>(Also, use p4 opened to
make sure you don't have any files checked out before
starting.)<br><br>JVS<br><br>_______________________________________________<br>Mondrian
mailing list<br><a href="mailto:Mondrian@pentaho.org" target="_blank"></a><a href="mailto:Mondrian@pentaho.org" target="_blank">Mondrian@pentaho.org</a><br><a href="http://lists.pentaho.org/mailman/listinfo/mondrian" target="_blank"></a><a href="http://lists.pentaho.org/mailman/listinfo/mondrian" target="_blank">http://lists.pentaho.org/mailman/listinfo/mondrian</a><br>
</blockquote></div><br></div></blockquote>
<blockquote type="cite">
<div><span>_______________________________________________</span><br><span>Mondrian
mailing list</span><br><span><a href="mailto:Mondrian@pentaho.org" target="_blank">Mondrian@pentaho.org</a></span><br><span><a href="http://lists.pentaho.org/mailman/listinfo/mondrian" target="_blank">http://lists.pentaho.org/mailman/listinfo/mondrian</a></span><br>
</div></blockquote></div></div></div><br>_______________________________________________<br>Mondrian
mailing list<br><a href="mailto:Mondrian@pentaho.org" target="_blank">Mondrian@pentaho.org</a><br><a href="http://lists.pentaho.org/mailman/listinfo/mondrian" target="_blank">http://lists.pentaho.org/mailman/listinfo/mondrian</a><br>
<br></blockquote></div><br></div></div></div></blockquote></div><br></div></div></div></div></div></div></blockquote></div><br></div></div></blockquote></div>
<br>_______________________________________________<br>
Mondrian mailing list<br>
<a href="mailto:Mondrian@pentaho.org">Mondrian@pentaho.org</a><br>
<a href="http://lists.pentaho.org/mailman/listinfo/mondrian" target="_blank">http://lists.pentaho.org/mailman/listinfo/mondrian</a><br>
<br></blockquote></div><br></div>