threads / patch / 12918

patch, 4 partsChange history page table to SWT.VIRTUAL.

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

## tl;dr

6 messages between Mar 30, 2008 and Apr 1, 2008. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Roger C. Soares· Mar 30, 2008, 15:18 UTC · lore

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(-)

Show changes to org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java +1 −1

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

Shawn O. Pearce· Mar 31, 2008, 05:34 UTC · re: Roger C. Soares · lore

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

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:

Show 10 quoted lines

> 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.

Show 13 quoted lines

> 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.

Roger C. Soares· Apr 1, 2008, 03:25 UTC · re: Shawn O. Pearce · lore

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

Shawn O. Pearce escreveu:

Show 7 quoted lines

> 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.

Shawn O. Pearce· Apr 1, 2008, 03:36 UTC · re: Roger C. Soares · lore

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

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:

Show 11 quoted lines

> 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.

Show 10 quoted lines

> >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.

Roger C. Soares· Apr 1, 2008, 03:54 UTC · re: Shawn O. Pearce · lore

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

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.

Show 12 quoted lines

>> 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.

Shawn O. Pearce· Apr 1, 2008, 04:12 UTC · re: Roger C. Soares · lore

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

"Roger C. Soares" <rogersoares@intelinet.com.br> wrote:

Show 12 quoted lines

>
> >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.