[EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[Roger C. Soares]

It makes the history page show about the same speed as gitk on my
eclipse.

>From the eclipse API:

"Style VIRTUAL is used to create a Table whose TableItems are to be
populated by the client on an on-demand basis instead of up-front.
This can provide significant performance improvements for tables
that are very large or for which TableItem population is expensive
(for example, retrieving values from an external source)."

Signed-off-by: Roger C. Soares <rogersoares@intelinet.com.br>
---
Hi Shawn, Robin,

This patch series is currently on top of 2392caa5a495f72ab25dee10709d98bb21a45ab9
from Shawn's repo.

I didn't apply the find in references patch because it depends on branch
information that is not available yet.

[]s,
Roger.


 .../egit/ui/internal/history/CommitGraphTable.java |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java
index fffe7e0..6559d64 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java
@@ -88,7 +88,7 @@ class CommitGraphTable {
 		hFont = highlightFont();
 
 		final Table rawTable = new Table(parent, SWT.MULTI | SWT.H_SCROLL
-				| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION);
+				| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION | SWT.VIRTUAL);
 		rawTable.setHeaderVisible(true);
 		rawTable.setLinesVisible(false);
 		rawTable.setFont(nFont);
-- 
1.5.4.1

Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[Shawn O. Pearce]

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:
> It makes the history page show about the same speed as gitk on my
> eclipse.
> 
> From the eclipse API:
> 
> "Style VIRTUAL is used to create a Table whose TableItems are to be
> populated by the client on an on-demand basis instead of up-front.
> This can provide significant performance improvements for tables
> that are very large or for which TableItem population is expensive
> (for example, retrieving values from an external source)."

Yea, I originally wrote my series around the VIRTUAL flag but on
Win32 it caused ArrayIndexOutOfBoundsExceptions to be thrown from
deep down within the Win32 implementation of the SWT Table widget.

Appears to be something of a known bug, based on the Eclipse issue
tracker, but not much work happening to fix it.

I'll retest this tomorrow on Win32, but I'm pretty certain its
a bad idea on that platform.  What are you running on, Linux?
Maybe we can set this flag everywhere except on Win32.
 
> diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java
> index fffe7e0..6559d64 100644
> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java
> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java
> @@ -88,7 +88,7 @@ class CommitGraphTable {
>  		hFont = highlightFont();
>  
>  		final Table rawTable = new Table(parent, SWT.MULTI | SWT.H_SCROLL
> -				| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION);
> +				| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION | SWT.VIRTUAL);
>  		rawTable.setHeaderVisible(true);
>  		rawTable.setLinesVisible(false);
>  		rawTable.setFont(nFont);

-- 
Shawn.

Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[Roger C. Soares]

Shawn O. Pearce escreveu:
> Yea, I originally wrote my series around the VIRTUAL flag but on
> Win32 it caused ArrayIndexOutOfBoundsExceptions to be thrown from
> deep down within the Win32 implementation of the SWT Table widget.
>
> Appears to be something of a known bug, based on the Eclipse issue
> tracker, but not much work happening to fix it.
>   
Hum, a VIRTUAL table sounds like a very usefull feature to be badly 
broken on windows, at least there should be some workaround... is it 
easily reproducible?


> I'll retest this tomorrow on Win32, but I'm pretty certain its
> a bad idea on that platform.  What are you running on, Linux?
> Maybe we can set this flag everywhere except on Win32
Yep, linux.

Maybe another option to try before leaving windows out is the 
ILazyContentProvider. Have you noticed that while GenerateHistoryJob is 
updating the table you can't use it? Because the input is regenerated 
every time, the table keeps going back to the first row.

[]s,
Roger.

Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[Shawn O. Pearce]

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:
> Shawn O. Pearce escreveu:
> >Yea, I originally wrote my series around the VIRTUAL flag but on
> >Win32 it caused ArrayIndexOutOfBoundsExceptions to be thrown from
> >deep down within the Win32 implementation of the SWT Table widget.
> >
> >Appears to be something of a known bug, based on the Eclipse issue
> >tracker, but not much work happening to fix it.
>
> Hum, a VIRTUAL table sounds like a very usefull feature to be badly 
> broken on windows, at least there should be some workaround... is it 
> easily reproducible?

I tested your patch this evening on Win32.  Its nearly instant.
Seriously.  gitk can't touch it on the same system.  The bug I
was seeing didn't happen.

Originally I wrote the graph table using the ILazyContentProvider
*and* the SWT.VIRTUAL flag.  I didn't realize you had the option to
set only the SWT.VIRTUAL flag.  I think the bug on Win32 is related
to how the ILazyContentProvider gets used by the JFace TableViewer
and less about the SWT.VIRTUAL flag itself.

So anyway, I see no breakage on Mac OS X or Win32 with your patch,
and you said you tested it on Linux, so I'm going to include it.
Thanks for figuring that one out, its a nice performance boost.

> >I'll retest this tomorrow on Win32, but I'm pretty certain its
> >a bad idea on that platform.  What are you running on, Linux?
> >Maybe we can set this flag everywhere except on Win32
>
> Yep, linux.
> 
> Maybe another option to try before leaving windows out is the 
> ILazyContentProvider. Have you noticed that while GenerateHistoryJob is 
> updating the table you can't use it? Because the input is regenerated 
> every time, the table keeps going back to the first row.

Hmm.  I didn't notice that, but at this point its so damn fast for
me that I don't have the reflexes to really try and use the table
before GenerateHistoryJob is complete.  I'll have to add some sleeps
in there to make it slow down its work and see if I can reproduce
what you are describing.

-- 
Shawn.

Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[Roger C. Soares]

Shawn O. Pearce escreveu:
> So anyway, I see no breakage on Mac OS X or Win32 with your patch,
> and you said you tested it on Linux, so I'm going to include it.
> Thanks for figuring that one out, its a nice performance boost.
>   
Cool.


>> Maybe another option to try before leaving windows out is the 
>> ILazyContentProvider. Have you noticed that while GenerateHistoryJob is 
>> updating the table you can't use it? Because the input is regenerated 
>> every time, the table keeps going back to the first row.
>>     
>
> Hmm.  I didn't notice that, but at this point its so damn fast for
> me that I don't have the reflexes to really try and use the table
> before GenerateHistoryJob is complete.  I'll have to add some sleeps
> in there to make it slow down its work and see if I can reproduce
> what you are describing.
>   
Yep, I noticed it while debuging and don't think it's very important. 
I'm very happy with the current speed and it's also almost instant to 
me. I think the lazy provider could be of some help in case someone is 
reading from a slow nfs partition or something like that. It's very 
border case thought.

[]s,
Roger.

Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.

[Shawn O. Pearce]

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:
>
> >Hmm.  I didn't notice that, but at this point its so damn fast for
> >me that I don't have the reflexes to really try and use the table
> >before GenerateHistoryJob is complete.  I'll have to add some sleeps
> >in there to make it slow down its work and see if I can reproduce
> >what you are describing.
>
> Yep, I noticed it while debuging and don't think it's very important. 
> I'm very happy with the current speed and it's also almost instant to 
> me. I think the lazy provider could be of some help in case someone is 
> reading from a slow nfs partition or something like that. It's very 
> border case thought.

Actually I think the trick to use there is the "early output
and restart" that C Git's rev-list and gitk learned about two
months back.  We don't do this in jgit yet and that means we have
to produce _all_ commits before we can topologically sort them and
return even the first commit.

We should be able to produce results immediately and then force
a reset and redraw of the table when we find the rare cases where
topological sorting gets violated by honoring the standard commit
date ordering.

There aren't many such cases in git.git or linux-2.6.git.  They only
happen if clock skew is enough that folks are able to create multiple
commits at the same time that are out of order according to topology.
And remember that in the GUI we do wind up redrawing the table anyway
as we add new items to the end of it.  So its really no big deal
to just reset everything and restart when we find the violations.

Its on my list of things to add to the jgit machinary, but I'm also
looking to get push[*1*] and merge implemented.  I am reasonably
happy with the performance of the History view, as its actually
now usuable on real projects.  So its time to add new features,
rather than optimizing old ones.


[*1*]  Hopefully this is a successful GSoC 2008 project.  ;-)

-- 
Shawn.