{"thread":{"id":"44430","subject":"gitk: avoid obscene memory consumption","startedAt":"2016-11-04T22:36:33Z","lastAt":"2016-11-07T13:43:44Z","messageCount":7,"participants":["Markus Hitter","Stefan Beller","Paul Mackerras","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"305418","messageId":"47c374cf-e6b9-8cd3-ee0d-d877e9e96a62@jump-ing.de","threadId":"44430","inReplyTo":null,"subject":"gitk: avoid obscene memory consumption","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-04T19:49:41Z","receivedAt":"2016-11-04T22:36:33Z","isPatch":false,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"\nHello all,\n\nafter Gitk brought my shabby development machine (Core2Duo, 4 GB RAM, Ubuntu 16.10, no swap to save the SSD) to its knees once more than I'm comfortable with, I decided to investigate this issue.\n\nResult of this investigation is, my Git repo has a commit with a diff of some 365'000 lines and Gitk tries to display all of them, consuming more than 1.5 GB of memory.\n\nThe solution is to cut off diffs at 50'000 lines for the display. This consumes about 350 MB RAM, still a lot. These first 50'000 lines are shown, followed by a copyable message on how to view the full diff on the command line. Diffs shorter than this limit are displayed as before.\n\nTo test the waters whether such a change is welcome, here's the patch as I currently use it. If this patch makes sense I'll happily apply change requests and bring it more in line with Git's patch submission expectations. The patch is made against git(k) version 2.9.3, the one coming with latest Ubuntu. Please also note that this is the first time I wrote some Tcl code, so the strategy used might not follow best Tcl practices.\n\n$ diff -uw /usr/bin/gitk.org /usr/bin/gitk\n--- /usr/bin/gitk.org\t2016-08-16 22:32:47.000000000 +0200\n+++ /usr/bin/gitk\t2016-11-04 20:06:14.805920404 +0100\n@@ -7,6 +7,15 @@\n # and distributed under the terms of the GNU General Public Licence,\n # either version 2, or (at your option) any later version.\n \n+# Markus: trying to limit memory consumption. It happened that\n+#         complex commits led to more than 1.5 GB of memory usage.\n+#\n+# The problem was identified to be caused by extremely long diffs. The\n+# commit leading to this research had some 365'000 lines of diff, consuming\n+# these 1.5 GB when drawn into the canvas. The solution is to limit diffs to\n+# 50'000 lines and skipping the rest. In case of a cutoff, a CLI command for\n+# getting the full diff is shown.\n+\n package require Tk\n \n proc hasworktree {} {\n@@ -7956,6 +7965,7 @@\n \n proc getblobdiffs {ids} {\n     global blobdifffd diffids env\n+    global parseddifflines\n     global treediffs\n     global diffcontext\n     global ignorespace\n@@ -7987,6 +7997,7 @@\n     }\n     fconfigure $bdf -blocking 0 -encoding binary -eofchar {}\n     set blobdifffd($ids) $bdf\n+    set parseddifflines 0\n     initblobdiffvars\n     filerun $bdf [list getblobdiffline $bdf $diffids]\n }\n@@ -8063,20 +8074,34 @@\n \n proc getblobdiffline {bdf ids} {\n     global diffids blobdifffd\n+    global parseddifflines\n     global ctext\n \n     set nr 0\n+    set maxlines 50000\n     $ctext conf -state normal\n     while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {\n+        incr parseddifflines\n+        if {$parseddifflines >= $maxlines} {\n+            break\n+        }\n \tif {$ids != $diffids || $bdf != $blobdifffd($ids)} {\n \t    catch {close $bdf}\n \t    return 0\n \t}\n \tparseblobdiffline $ids $line\n     }\n+    if {$parseddifflines >= $maxlines} {\n+        $ctext insert end \"\\n------------------\" hunksep\n+        $ctext insert end \" Lines exceeding $maxlines skipped \" hunksep\n+        $ctext insert end \"------------------\\n\\n\" hunksep\n+        $ctext insert end \"To get a full diff, run\\n\\n\" hunksep\n+        $ctext insert end \"  git diff-tree -p -C --cc $ids\\n\\n\" hunksep\n+        $ctext insert end \"on the command line.\\n\" hunksep\n+    }\n     $ctext conf -state disabled\n     blobdiffmaybeseehere [eof $bdf]\n-    if {[eof $bdf]} {\n+    if {[eof $bdf] || $parseddifflines >= $maxlines} {\n \tcatch {close $bdf}\n \treturn 0\n     }\n@@ -9093,6 +9118,7 @@\n \n proc diffcommits {a b} {\n     global diffcontext diffids blobdifffd diffinhdr currdiffsubmod\n+    global parseddifflines\n \n     set tmpdir [gitknewtmpdir]\n     set fna [file join $tmpdir \"commit-[string range $a 0 7]\"]\n@@ -9114,6 +9140,7 @@\n     set blobdifffd($diffids) $fd\n     set diffinhdr 0\n     set currdiffsubmod \"\"\n+    set parseddifflines 0\n     filerun $fd [list getblobdiffline $fd $diffids]\n }\n \n\nCheers,\nMarkus\n\n-- \n- - - - - - - - - - - - - - - - - - -\nDipl. Ing. (FH) Markus Hitter\nhttp://www.jump-ing.de/\n"},{"id":"305420","messageId":"CAGZ79kbavzGJ2sAcz5heg+BO+tZ=TgtrhxMH1-kqeJUpNNavyw@mail.gmail.com","threadId":"44430","inReplyTo":"47c374cf-e6b9-8cd3-ee0d-d877e9e96a62@jump-ing.de","subject":"Re: gitk: avoid obscene memory consumption","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-04T22:45:09Z","receivedAt":"2016-11-04T22:45:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 4, 2016 at 12:49 PM, Markus Hitter <mah@jump-ing.de> wrote:\n>\n> Hello all,\n\n+cc Paul Mackeras, who maintains gitk.\n\n>\n> after Gitk brought my shabby development machine (Core2Duo, 4 GB RAM, Ubuntu 16.10, no swap to save the SSD) to its knees once more than I'm comfortable with, I decided to investigate this issue.\n>\n> Result of this investigation is, my Git repo has a commit with a diff of some 365'000 lines and Gitk tries to display all of them, consuming more than 1.5 GB of memory.\n>\n> The solution is to cut off diffs at 50'000 lines for the display. This consumes about 350 MB RAM, still a lot. These first 50'000 lines are shown, followed by a copyable message on how to view the full diff on the command line. Diffs shorter than this limit are displayed as before.\n\nBikeshedding: I'd argue to even lower the number to 5-10k lines.\n\n>\n> To test the waters whether such a change is welcome, here's the patch as I currently use it. If this patch makes sense I'll happily apply change requests and bring it more in line with Git's patch submission expectations.\n\nI have never contributed to gitk myself,\nwhich is hosted at git://ozlabs.org/~paulus/gitk\nthough I'd expect these guide lines would roughly apply:\nhttps://github.com/git/git/blob/master/Documentation/SubmittingPatches\n"},{"id":"305442","messageId":"20161105110845.GA4039@fergus.ozlabs.ibm.com","threadId":"44430","inReplyTo":"CAGZ79kbavzGJ2sAcz5heg+BO+tZ=TgtrhxMH1-kqeJUpNNavyw@mail.gmail.com","subject":"Re: gitk: avoid obscene memory consumption","fromName":"Paul Mackerras","fromEmail":"paulus@ozlabs.org","sentAt":"2016-11-05T11:08:45Z","receivedAt":"2016-11-05T11:09:02Z","isPatch":false,"sender":{"key":"paulus@ozlabs.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Fri, Nov 04, 2016 at 03:45:09PM -0700, Stefan Beller wrote:\n> On Fri, Nov 4, 2016 at 12:49 PM, Markus Hitter <mah@jump-ing.de> wrote:\n> >\n> > Hello all,\n> \n> +cc Paul Mackeras, who maintains gitk.\n\nThanks.\n\n> >\n> > after Gitk brought my shabby development machine (Core2Duo, 4 GB RAM, Ubuntu 16.10, no swap to save the SSD) to its knees once more than I'm comfortable with, I decided to investigate this issue.\n> >\n> > Result of this investigation is, my Git repo has a commit with a diff of some 365'000 lines and Gitk tries to display all of them, consuming more than 1.5 GB of memory.\n> >\n> > The solution is to cut off diffs at 50'000 lines for the display. This consumes about 350 MB RAM, still a lot. These first 50'000 lines are shown, followed by a copyable message on how to view the full diff on the command line. Diffs shorter than this limit are displayed as before.\n\nThat sounds reasonable.\n\n> \n> Bikeshedding: I'd argue to even lower the number to 5-10k lines.\n\nI could go with 10k.\n\n> \n> >\n> > To test the waters whether such a change is welcome, here's the patch as I currently use it. If this patch makes sense I'll happily apply change requests and bring it more in line with Git's patch submission expectations.\n> \n> I have never contributed to gitk myself,\n> which is hosted at git://ozlabs.org/~paulus/gitk\n> though I'd expect these guide lines would roughly apply:\n> https://github.com/git/git/blob/master/Documentation/SubmittingPatches\n\nPaul.\n"},{"id":"305451","messageId":"ff5bb36b-e30c-3998-100d-789b4b5e7249@jump-ing.de","threadId":"44430","inReplyTo":"20161105110845.GA4039@fergus.ozlabs.ibm.com","subject":"Re: gitk: avoid obscene memory consumption","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-06T10:28:37Z","receivedAt":"2016-11-06T10:28:49Z","isPatch":false,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"Am 05.11.2016 um 12:08 schrieb Paul Mackerras:\n> On Fri, Nov 04, 2016 at 03:45:09PM -0700, Stefan Beller wrote:\n>> On Fri, Nov 4, 2016 at 12:49 PM, Markus Hitter <mah@jump-ing.de> wrote:\n>>>\n>>> Hello all,\n>>\n>> +cc Paul Mackeras, who maintains gitk.\n> \n> Thanks.\n> \n>>>\n>>> after Gitk brought my shabby development machine (Core2Duo, 4 GB RAM, Ubuntu 16.10, no swap to save the SSD) to its knees once more than I'm comfortable with, I decided to investigate this issue.\n>>>\n>>> Result of this investigation is, my Git repo has a commit with a diff of some 365'000 lines and Gitk tries to display all of them, consuming more than 1.5 GB of memory.\n>>>\n>>> The solution is to cut off diffs at 50'000 lines for the display. This consumes about 350 MB RAM, still a lot. These first 50'000 lines are shown, followed by a copyable message on how to view the full diff on the command line. Diffs shorter than this limit are displayed as before.\n> \n> That sounds reasonable.\n> \n>>\n>> Bikeshedding: I'd argue to even lower the number to 5-10k lines.\n> \n> I could go with 10k.\n\nThanks for the positive comments.\n\nTBH, the more I think about the problem, the less I'm satisfied with the solution I provided. Including two reasons:\n\n- The list of files affected to the right is still complete and clicking a file name further down results in nothing ... as if the file wasn't part of the diff.\n\n- Local searches. Cutting off diffs makes them unreliable. Global searches still work, but actually viewing a search result in the skipped section is no longer possible.\n\nSo I'm watching out for better solutions. So far I can think of these:\n\n- Storing only the actually viewed diff. It's an interactive tool, so there's no advantage in displaying the diff in 0.001 seconds over viewing it in 0.1 seconds. As far as I can see, Gitk currently stores every diff it gets a hold of forever.\n\n- View the diff sparsely. Like rendering only the actually visible portion.\n\n- Enhancing ctext. This reference diff has 28 million characters, so there should be a way to store this with color information in, let's say, 29 MB of memory.\n\nAny additional ideas?\n\n\nMarkus\n\n-- \n- - - - - - - - - - - - - - - - - - -\nDipl. Ing. (FH) Markus Hitter\nhttp://www.jump-ing.de/\n"},{"id":"305469","messageId":"CA+P7+xo6qTjf3R1WTjyRUAn0-2pyKXRpf=v_aMJGPXQg39SweA@mail.gmail.com","threadId":"44430","inReplyTo":"ff5bb36b-e30c-3998-100d-789b4b5e7249@jump-ing.de","subject":"Re: gitk: avoid obscene memory consumption","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-11-06T20:33:19Z","receivedAt":"2016-11-06T20:42:21Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Nov 6, 2016 at 2:28 AM, Markus Hitter <mah@jump-ing.de> wrote:\n> - Storing only the actually viewed diff. It's an interactive tool, so there's no advantage in displaying the diff in 0.001 seconds over viewing it in 0.1 seconds. As far as I can see, Gitk currently stores every diff it gets a hold of forever.\n>\n\nThis seems like the right solution. Store only what we need to view as\nwe need to view it. (IE: lazily generate the diff and don't keep it\nlong term, possibly by generating each file separately when that file\nis viewed)?\n\n> - View the diff sparsely. Like rendering only the actually visible portion.\n>\n\nThis also would be valuable, as part of the solution above.\n\n> - Enhancing ctext. This reference diff has 28 million characters, so there should be a way to store this with color information in, let's say, 29 MB of memory.\n>\n\nI think all three suggestions here are a better solution that what\nyou've outlined already as you explain the problems caused by cutting\noff the diff.\n\nThanks,\nJake\n"},{"id":"305475","messageId":"20161107041138.rnlzyuacoezsfwif@oak.ozlabs.ibm.com","threadId":"44430","inReplyTo":"ff5bb36b-e30c-3998-100d-789b4b5e7249@jump-ing.de","subject":"Re: gitk: avoid obscene memory consumption","fromName":"Paul Mackerras","fromEmail":"paulus@ozlabs.org","sentAt":"2016-11-07T04:11:38Z","receivedAt":"2016-11-07T04:11:46Z","isPatch":false,"sender":{"key":"paulus@ozlabs.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Sun, Nov 06, 2016 at 11:28:37AM +0100, Markus Hitter wrote:\n> \n> Thanks for the positive comments.\n> \n> TBH, the more I think about the problem, the less I'm satisfied with the solution I provided. Including two reasons:\n> \n> - The list of files affected to the right is still complete and clicking a file name further down results in nothing ... as if the file wasn't part of the diff.\n> \n> - Local searches. Cutting off diffs makes them unreliable. Global searches still work, but actually viewing a search result in the skipped section is no longer possible.\n> \n> So I'm watching out for better solutions. So far I can think of these:\n> \n> - Storing only the actually viewed diff. It's an interactive tool, so there's no advantage in displaying the diff in 0.001 seconds over viewing it in 0.1 seconds. As far as I can see, Gitk currently stores every diff it gets a hold of forever.\n\nIt does?  That would be a bug. :)\n\n> \n> - View the diff sparsely. Like rendering only the actually visible portion.\n> \n> - Enhancing ctext. This reference diff has 28 million characters, so there should be a way to store this with color information in, let's say, 29 MB of memory.\n\nTcl uses Unicode internally, I believe, so 57MB, but yes.\n\nPaul.\n"},{"id":"305484","messageId":"3b16a0f5-46e3-b41c-553a-473ad3e9cf26@jump-ing.de","threadId":"44430","inReplyTo":"20161107041138.rnlzyuacoezsfwif@oak.ozlabs.ibm.com","subject":"Re: gitk: avoid obscene memory consumption","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-07T13:43:04Z","receivedAt":"2016-11-07T13:43:44Z","isPatch":false,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"Am 07.11.2016 um 05:11 schrieb Paul Mackerras:\n>> - Storing only the actually viewed diff. It's an interactive tool, so there's no advantage in displaying the diff in 0.001 seconds over viewing it in 0.1 seconds. As far as I can see, Gitk currently stores every diff it gets a hold of forever.\n> It does?  That would be a bug. :)\n> \n\nSo far I've found three arrays being populated lazily (which is good) but never being released (which ignores changes to the underlying repo):\n\n$commitinfo: one entry of about 500 bytes per line viewed in the list of commits. Maximum size of the array is the number of commits. As far as I can see, this array should be removed on a reload (Shift-F5).\n\n$blobdifffd: one entry of about 45 bytes for every commit ever read. The underlying file descriptor gets closed, but the entry in this array remains. So far I didn't find the reason why this array exists at all. It's also not removed on a reload.\n\n$treediffs: always the same number of entries as $blobdiffd, but > 1000 bytes/entry. Removed/refreshed on a reload (good!), different number of entries from that point on.\n\nIn case you want to play as well, here's the code I wrote for the investigation, it can be appended right at the bottom of the gitk script:\n\n--------------8<---------------\nproc variableSizes {} {\n    # Add variable here to get them shown.\n    global diffcontext diffids blobdifffd currdiffsubmod commitinfo\n    global diffnexthead diffnextnote difffilestart\n    global diffinhdr treediffs\n\n    puts \"---------------------------------------------------\"\n    foreach V [info vars] {\n\tif { ! [info exists $V] } {\n\t    continue\n\t}\n\n\tset count 0\n\tset bytes 0\n\tif [array exists $V] {\n\t    set count [array size $V]\n\t    foreach I [array get $V] {\n\t\tset bytes [expr $bytes + [string bytelength $I]]\n\t    }\n\t} elseif [catch {llength [set $V]}] {\n\t    set count [llength [set $V]]\n#\t    set bytes [string bytelength [list {*}[set $V]]]\n\t} else {\n\t    set bytes [string bytelength [set $V]]\n\t}\n\tputs [format \"%20s: %5d items, %10d bytes\" $V $count $bytes]\n    }\n\n#    catch {\n#\tset output [memory info]\n#\tputs $output\n#    }\n\n    after 3000 variableSizes\n}\n\nvariableSizes\n-------------->8---------------\n\n[memory info] requires a Tcl with memory debug enabled.\n\n\nMarkus\n-- \n- - - - - - - - - - - - - - - - - - -\nDipl. Ing. (FH) Markus Hitter\nhttp://www.jump-ing.de/\n"}]}