{"thread":{"id":"12918","subject":"[EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","startedAt":"2008-03-30T15:18:34Z","lastAt":"2008-04-01T04:12:03Z","messageCount":6,"participants":["Roger C. Soares","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"73355","messageId":"1206890314-3712-1-git-send-email-rogersoares@intelinet.com.br","threadId":"12918","inReplyTo":null,"subject":"[EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","fromName":"Roger C. Soares","fromEmail":"rogersoares@intelinet.com.br","sentAt":"2008-03-30T15:18:34Z","receivedAt":"2008-03-30T15:18:34Z","isPatch":true,"sender":{"key":"rogersoares@intelinet.com.br","avatar":null},"body":"It makes the history page show about the same speed as gitk on my\neclipse.\n\n>From the eclipse API:\n\n\"Style VIRTUAL is used to create a Table whose TableItems are to be\npopulated by the client on an on-demand basis instead of up-front.\nThis can provide significant performance improvements for tables\nthat are very large or for which TableItem population is expensive\n(for example, retrieving values from an external source).\"\n\nSigned-off-by: Roger C. Soares \n---\nHi Shawn, Robin,\n\nThis patch series is currently on top of 2392caa5a495f72ab25dee10709d98bb21a45ab9\nfrom Shawn's repo.\n\nI didn't apply the find in references patch because it depends on branch\ninformation that is not available yet.\n\n[]s,\nRoger.\n\n\n .../egit/ui/internal/history/CommitGraphTable.java | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --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\nindex fffe7e0..6559d64 100644\n--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java\n+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java\n@@ -88,7 +88,7 @@ class CommitGraphTable {\n \t\thFont = highlightFont();\n \n \t\tfinal Table rawTable = new Table(parent, SWT.MULTI | SWT.H_SCROLL\n-\t\t\t\t| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION);\n+\t\t\t\t| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION | SWT.VIRTUAL);\n \t\trawTable.setHeaderVisible(true);\n \t\trawTable.setLinesVisible(false);\n \t\trawTable.setFont(nFont);\n-- \n1.5.4.1\n"},{"id":"73409","messageId":"20080331053430.GJ10274@spearce.org","threadId":"12918","inReplyTo":"1206890314-3712-1-git-send-email-rogersoares@intelinet.com.br","subject":"Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-03-31T05:34:30Z","receivedAt":"2008-03-31T05:34:30Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Roger C. Soares\" wrote:\n> It makes the history page show about the same speed as gitk on my\n> eclipse.\n> \n> From the eclipse API:\n> \n> \"Style VIRTUAL is used to create a Table whose TableItems are to be\n> populated by the client on an on-demand basis instead of up-front.\n> This can provide significant performance improvements for tables\n> that are very large or for which TableItem population is expensive\n> (for example, retrieving values from an external source).\"\n\nYea, I originally wrote my series around the VIRTUAL flag but on\nWin32 it caused ArrayIndexOutOfBoundsExceptions to be thrown from\ndeep down within the Win32 implementation of the SWT Table widget.\n\nAppears to be something of a known bug, based on the Eclipse issue\ntracker, but not much work happening to fix it.\n\nI'll retest this tomorrow on Win32, but I'm pretty certain its\na bad idea on that platform. What are you running on, Linux?\nMaybe we can set this flag everywhere except on Win32.\n \n> 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\n> index fffe7e0..6559d64 100644\n> --- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java\n> +++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/history/CommitGraphTable.java\n> @@ -88,7 +88,7 @@ class CommitGraphTable {\n> \t\thFont = highlightFont();\n> \n> \t\tfinal Table rawTable = new Table(parent, SWT.MULTI | SWT.H_SCROLL\n> -\t\t\t\t| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION);\n> +\t\t\t\t| SWT.V_SCROLL | SWT.BORDER | SWT.FULL_SELECTION | SWT.VIRTUAL);\n> \t\trawTable.setHeaderVisible(true);\n> \t\trawTable.setLinesVisible(false);\n> \t\trawTable.setFont(nFont);\n\n-- \nShawn.\n"},{"id":"73470","messageId":"47F1AB1B.90309@intelinet.com.br","threadId":"12918","inReplyTo":"20080331053430.GJ10274@spearce.org","subject":"Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","fromName":"Roger C. Soares","fromEmail":"rogersoares@intelinet.com.br","sentAt":"2008-04-01T03:25:15Z","receivedAt":"2008-04-01T03:25:15Z","isPatch":true,"sender":{"key":"rogersoares@intelinet.com.br","avatar":null},"body":"\nShawn O. Pearce escreveu:\n> Yea, I originally wrote my series around the VIRTUAL flag but on\n> Win32 it caused ArrayIndexOutOfBoundsExceptions to be thrown from\n> deep down within the Win32 implementation of the SWT Table widget.\n>\n> Appears to be something of a known bug, based on the Eclipse issue\n> tracker, but not much work happening to fix it.\n> \nHum, a VIRTUAL table sounds like a very usefull feature to be badly \nbroken on windows, at least there should be some workaround... is it \neasily reproducible?\n\n\n> I'll retest this tomorrow on Win32, but I'm pretty certain its\n> a bad idea on that platform. What are you running on, Linux?\n> Maybe we can set this flag everywhere except on Win32\nYep, linux.\n\nMaybe another option to try before leaving windows out is the \nILazyContentProvider. Have you noticed that while GenerateHistoryJob is \nupdating the table you can't use it? Because the input is regenerated \nevery time, the table keeps going back to the first row.\n\n[]s,\nRoger.\n"},{"id":"73471","messageId":"20080401033614.GP10274@spearce.org","threadId":"12918","inReplyTo":"47F1AB1B.90309@intelinet.com.br","subject":"Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-04-01T03:36:14Z","receivedAt":"2008-04-01T03:36:14Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Roger C. Soares\" wrote:\n> Shawn O. Pearce escreveu:\n> >Yea, I originally wrote my series around the VIRTUAL flag but on\n> >Win32 it caused ArrayIndexOutOfBoundsExceptions to be thrown from\n> >deep down within the Win32 implementation of the SWT Table widget.\n> >\n> >Appears to be something of a known bug, based on the Eclipse issue\n> >tracker, but not much work happening to fix it.\n>\n> Hum, a VIRTUAL table sounds like a very usefull feature to be badly \n> broken on windows, at least there should be some workaround... is it \n> easily reproducible?\n\nI tested your patch this evening on Win32. Its nearly instant.\nSeriously. gitk can't touch it on the same system. The bug I\nwas seeing didn't happen.\n\nOriginally I wrote the graph table using the ILazyContentProvider\n*and* the SWT.VIRTUAL flag. I didn't realize you had the option to\nset only the SWT.VIRTUAL flag. I think the bug on Win32 is related\nto how the ILazyContentProvider gets used by the JFace TableViewer\nand less about the SWT.VIRTUAL flag itself.\n\nSo anyway, I see no breakage on Mac OS X or Win32 with your patch,\nand you said you tested it on Linux, so I'm going to include it.\nThanks for figuring that one out, its a nice performance boost.\n\n> >I'll retest this tomorrow on Win32, but I'm pretty certain its\n> >a bad idea on that platform. What are you running on, Linux?\n> >Maybe we can set this flag everywhere except on Win32\n>\n> Yep, linux.\n> \n> Maybe another option to try before leaving windows out is the \n> ILazyContentProvider. Have you noticed that while GenerateHistoryJob is \n> updating the table you can't use it? Because the input is regenerated \n> every time, the table keeps going back to the first row.\n\nHmm. I didn't notice that, but at this point its so damn fast for\nme that I don't have the reflexes to really try and use the table\nbefore GenerateHistoryJob is complete. I'll have to add some sleeps\nin there to make it slow down its work and see if I can reproduce\nwhat you are describing.\n\n-- \nShawn.\n"},{"id":"73473","messageId":"47F1B1D9.3090209@intelinet.com.br","threadId":"12918","inReplyTo":"20080401033614.GP10274@spearce.org","subject":"Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","fromName":"Roger C. Soares","fromEmail":"rogersoares@intelinet.com.br","sentAt":"2008-04-01T03:54:01Z","receivedAt":"2008-04-01T03:54:01Z","isPatch":true,"sender":{"key":"rogersoares@intelinet.com.br","avatar":null},"body":"\nShawn O. Pearce escreveu:\n> So anyway, I see no breakage on Mac OS X or Win32 with your patch,\n> and you said you tested it on Linux, so I'm going to include it.\n> Thanks for figuring that one out, its a nice performance boost.\n> \nCool.\n\n\n>> Maybe another option to try before leaving windows out is the \n>> ILazyContentProvider. Have you noticed that while GenerateHistoryJob is \n>> updating the table you can't use it? Because the input is regenerated \n>> every time, the table keeps going back to the first row.\n>> \n>\n> Hmm. I didn't notice that, but at this point its so damn fast for\n> me that I don't have the reflexes to really try and use the table\n> before GenerateHistoryJob is complete. I'll have to add some sleeps\n> in there to make it slow down its work and see if I can reproduce\n> what you are describing.\n> \nYep, I noticed it while debuging and don't think it's very important. \nI'm very happy with the current speed and it's also almost instant to \nme. I think the lazy provider could be of some help in case someone is \nreading from a slow nfs partition or something like that. It's very \nborder case thought.\n\n[]s,\nRoger.\n"},{"id":"73482","messageId":"20080401041203.GR10274@spearce.org","threadId":"12918","inReplyTo":"47F1B1D9.3090209@intelinet.com.br","subject":"Re: [EGIT PATCH 1/4] Change history page table to SWT.VIRTUAL.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-04-01T04:12:03Z","receivedAt":"2008-04-01T04:12:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Roger C. Soares\" wrote:\n>\n> >Hmm. I didn't notice that, but at this point its so damn fast for\n> >me that I don't have the reflexes to really try and use the table\n> >before GenerateHistoryJob is complete. I'll have to add some sleeps\n> >in there to make it slow down its work and see if I can reproduce\n> >what you are describing.\n>\n> Yep, I noticed it while debuging and don't think it's very important. \n> I'm very happy with the current speed and it's also almost instant to \n> me. I think the lazy provider could be of some help in case someone is \n> reading from a slow nfs partition or something like that. It's very \n> border case thought.\n\nActually I think the trick to use there is the \"early output\nand restart\" that C Git's rev-list and gitk learned about two\nmonths back. We don't do this in jgit yet and that means we have\nto produce _all_ commits before we can topologically sort them and\nreturn even the first commit.\n\nWe should be able to produce results immediately and then force\na reset and redraw of the table when we find the rare cases where\ntopological sorting gets violated by honoring the standard commit\ndate ordering.\n\nThere aren't many such cases in git.git or linux-2.6.git. They only\nhappen if clock skew is enough that folks are able to create multiple\ncommits at the same time that are out of order according to topology.\nAnd remember that in the GUI we do wind up redrawing the table anyway\nas we add new items to the end of it. So its really no big deal\nto just reset everything and restart when we find the violations.\n\nIts on my list of things to add to the jgit machinary, but I'm also\nlooking to get push[*1*] and merge implemented. I am reasonably\nhappy with the performance of the History view, as its actually\nnow usuable on real projects. So its time to add new features,\nrather than optimizing old ones.\n\n\n[*1*] Hopefully this is a successful GSoC 2008 project. ;-)\n\n-- \nShawn.\n"}]}