{"thread":{"id":"44438","subject":"[PATCH 0/3] gitk: memory consumption improvements","startedAt":"2016-11-07T18:55:03Z","lastAt":"2016-12-12T09:51:51Z","messageCount":9,"participants":["Markus Hitter","Jacob Keller","Junio C Hamano","Paul Mackerras"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"305498","messageId":"de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de","threadId":"44438","inReplyTo":null,"subject":"[PATCH 0/3] gitk: memory consumption improvements","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-07T18:54:28Z","receivedAt":"2016-11-07T18:55:03Z","isPatch":true,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"\nList, Paul,\n\nafter searching for a while on why Gitk sometimes consumes exorbitant amounts of memory I found a pair of minor issues and also a big one: the text widget comes with an unlimited undo manager, which is turned on be default. Considering that each line is inserted seperately, this piles up a huuuge undo stack ... for a read-only text widget. Simply turning off this undo manager saves about 95% of memory when viewing large commits (with tens of thousands of diff lines).\n\n3 patches are about to follow:\n\n - turn off the undo manager,\n\n - forget already closed file descriptors and\n\n - forget the 'commitinfo' array on a reload to enforce reloading it.\n\nI hope this finds you appreciation.\n\n\nMarkus\n\n-- \n- - - - - - - - - - - - - - - - - - -\nDipl. Ing. (FH) Markus Hitter\nhttp://www.jump-ing.de/\n"},{"id":"305499","messageId":"e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de","threadId":"44438","inReplyTo":"de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de","subject":"[PATCH 1/3] gitk: turn off undo manager in the text widget","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-07T18:57:41Z","receivedAt":"2016-11-07T19:00:26Z","isPatch":true,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"From e965e1deb9747bbc2b40dc2de95afb65aee9f7fd Mon Sep 17 00:00:00 2001\nFrom: Markus Hitter <mah@jump-ing.de>\nDate: Sun, 6 Nov 2016 20:38:03 +0100\nSubject: [PATCH 1/3] gitk: turn off undo manager in the text widget\n\nThe diff text widget is read-only, so there's zero point in\nbuilding an undo stack. This change reduces memory consumption of\nthis widget by about 95%.\n\nMemory usage of the whole program for viewing a reference commit\nbefore; 579'692'744 bytes, after: 32'724'446 bytes.\n\nTest procedure:\n\n - Choose a largish commit and check it out. In this case one with\n   90'802 lines, 5'006'902 bytes.\n\n - Have a Tcl version with memory debugging enabled. This is,\n   build one with --enable-symbols=mem passed to configure.\n\n - Instrument Gitk to regularly show a memory dump. E.g. by adding\n   these code lines at the very bottom:\n\n     proc memDump {} {\n         catch {\n             set output [memory info]\n             puts $output\n         }\n\n         after 3000 memDump\n     }\n\n     memDump\n\n - Start Gitk, it'll load this largish commit into the diff text\n   field automatically (because it's the current commit).\n\n - Wait until memory consumption levels out and note the numbers.\n\nNote that the numbers reported by [memory info] are much smaller\nthan the ones reported in 'top' (1.75 GB vs. 105 MB in this case),\nlikely due to all the instrumentation coming with the debug\nversion of Tcl.\n\nSigned-off-by: Markus Hitter <mah@jump-ing.de>\n---\n gitk | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gitk b/gitk\nindex 805a1c7..8654e29 100755\n--- a/gitk\n+++ b/gitk\n@@ -2403,7 +2403,7 @@ proc makewindow {} {\n \n     set ctext .bleft.bottom.ctext\n     text $ctext -background $bgcolor -foreground $fgcolor \\\n-\t-state disabled -font textfont \\\n+\t-state disabled -undo 0 -font textfont \\\n \t-yscrollcommand scrolltext -wrap none \\\n \t-xscrollcommand \".bleft.bottom.sbhorizontal set\"\n     if {$have_tk85} {\n-- \n2.9.3\n\n"},{"id":"305500","messageId":"8e1c5923-d2a6-bc77-97ab-3f154b41d2ea@jump-ing.de","threadId":"44438","inReplyTo":"e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de","subject":"[PATCH 2/3] gitk: remove closed file descriptors from $blobdifffd","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-07T19:01:58Z","receivedAt":"2016-11-07T19:02:26Z","isPatch":true,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"From 0a463fcd977dc9558835c373e24a095e35ca3c82 Mon Sep 17 00:00:00 2001\nFrom: Markus Hitter <mah@jump-ing.de>\nDate: Mon, 7 Nov 2016 16:01:17 +0100\nSubject: [PATCH 2/3] gitk: remove closed file descriptors from $blobdifffd\n\nOne shouldn't have descriptors of already closed files around.\n\nThe first idea to deal with this (previously) ever growing array\nwas to remove it entirely, but it's needed to detect start of a\nnew diff with ths old diff not yet done. This happens when a user\nclicks on the same commit in the commit list repeatedly without\ndelay.\n\nSigned-off-by: Markus Hitter <mah@jump-ing.de>\n---\n gitk | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/gitk b/gitk\nindex 8654e29..518a4ce 100755\n--- a/gitk\n+++ b/gitk\n@@ -8069,7 +8069,11 @@ proc getblobdiffline {bdf ids} {\n     $ctext conf -state normal\n     while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {\n \tif {$ids != $diffids || $bdf != $blobdifffd($ids)} {\n+\t    # Older diff read. Abort it.\n \t    catch {close $bdf}\n+\t    if {$ids != $diffids} {\n+\t\tarray unset blobdifffd $ids\n+\t    }\n \t    return 0\n \t}\n \tparseblobdiffline $ids $line\n@@ -8078,6 +8082,7 @@ proc getblobdiffline {bdf ids} {\n     blobdiffmaybeseehere [eof $bdf]\n     if {[eof $bdf]} {\n \tcatch {close $bdf}\n+\tarray unset blobdifffd $ids\n \treturn 0\n     }\n     return [expr {$nr >= 1000? 2: 1}]\n-- \n2.9.3\n\n"},{"id":"305501","messageId":"2cb7f76f-0004-a5b6-79f1-9bb4f979cf14@jump-ing.de","threadId":"44438","inReplyTo":"8e1c5923-d2a6-bc77-97ab-3f154b41d2ea@jump-ing.de","subject":"[PATCH 3/3] gitk: clear array 'commitinfo' on reload","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-07T19:03:17Z","receivedAt":"2016-11-07T19:03:24Z","isPatch":true,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"From 8359452f426c68cc02250f25f20eaaacd2ddd001 Mon Sep 17 00:00:00 2001\nFrom: Markus Hitter <mah@jump-ing.de>\nDate: Mon, 7 Nov 2016 19:02:51 +0100\nSubject: [PATCH 3/3] gitk: clear array 'commitinfo' on reload\n\nAfter a reload we might have an entirely different set of commits,\nso keeping all of them leaks memory. Remove them all because\nre-creating them is not more expensive than testing wether they're\nstill valid. Lazy (re-)creation is already well established, so\na missing entry can't cause harm.\n\nSigned-off-by: Markus Hitter <mah@jump-ing.de>\n---\n gitk | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/gitk b/gitk\nindex 518a4ce..aef6db6 100755\n--- a/gitk\n+++ b/gitk\n@@ -588,7 +588,7 @@ proc updatecommits {} {\n proc reloadcommits {} {\n     global curview viewcomplete selectedline currentid thickerline\n     global showneartags treediffs commitinterest cached_commitrow\n-    global targetid\n+    global targetid commitinfo\n \n     set selid {}\n     if {$selectedline ne {}} {\n@@ -609,6 +609,7 @@ proc reloadcommits {} {\n \tgetallcommits\n     }\n     clear_display\n+    unset -nocomplain commitinfo\n     unset -nocomplain commitinterest\n     unset -nocomplain cached_commitrow\n     unset -nocomplain targetid\n-- \n2.9.3\n\n"},{"id":"305518","messageId":"CA+P7+xrSb0bEC4dvEXKGLdhnunO9oyU685t6VCwd0Sj-pnOT0w@mail.gmail.com","threadId":"44438","inReplyTo":"e09a5309-351d-d246-d272-f527f50ad444@jump-ing.de","subject":"Re: [PATCH 1/3] gitk: turn off undo manager in the text widget","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-11-07T22:13:38Z","receivedAt":"2016-11-07T22:14:04Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Nov 7, 2016 at 10:57 AM, Markus Hitter <mah@jump-ing.de> wrote:\n> From e965e1deb9747bbc2b40dc2de95afb65aee9f7fd Mon Sep 17 00:00:00 2001\n> From: Markus Hitter <mah@jump-ing.de>\n> Date: Sun, 6 Nov 2016 20:38:03 +0100\n> Subject: [PATCH 1/3] gitk: turn off undo manager in the text widget\n>\n> The diff text widget is read-only, so there's zero point in\n> building an undo stack. This change reduces memory consumption of\n> this widget by about 95%.\n>\n> Memory usage of the whole program for viewing a reference commit\n> before; 579'692'744 bytes, after: 32'724'446 bytes.\n>\n\nWow. Nice find!\n\n> Test procedure:\n>\n>  - Choose a largish commit and check it out. In this case one with\n>    90'802 lines, 5'006'902 bytes.\n>\n>  - Have a Tcl version with memory debugging enabled. This is,\n>    build one with --enable-symbols=mem passed to configure.\n>\n>  - Instrument Gitk to regularly show a memory dump. E.g. by adding\n>    these code lines at the very bottom:\n>\n>      proc memDump {} {\n>          catch {\n>              set output [memory info]\n>              puts $output\n>          }\n>\n>          after 3000 memDump\n>      }\n>\n>      memDump\n>\n>  - Start Gitk, it'll load this largish commit into the diff text\n>    field automatically (because it's the current commit).\n>\n>  - Wait until memory consumption levels out and note the numbers.\n>\n> Note that the numbers reported by [memory info] are much smaller\n> than the ones reported in 'top' (1.75 GB vs. 105 MB in this case),\n> likely due to all the instrumentation coming with the debug\n> version of Tcl.\n>\n\nStill, this is definitely the lions share of the memory issue.\nAdditionally, this fix seems much better overall and does not harm any\nother aspects of gitk, because we only read the widget so there is as\nyou mentioned, zero reason to build an undo stack.\n\nThanks for taking the extra time to find a proper solution to this! I\nthink it makes perfect sense.\n\n> Signed-off-by: Markus Hitter <mah@jump-ing.de>\n> ---\n>  gitk | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/gitk b/gitk\n> index 805a1c7..8654e29 100755\n> --- a/gitk\n> +++ b/gitk\n> @@ -2403,7 +2403,7 @@ proc makewindow {} {\n>\n>      set ctext .bleft.bottom.ctext\n>      text $ctext -background $bgcolor -foreground $fgcolor \\\n> -       -state disabled -font textfont \\\n> +       -state disabled -undo 0 -font textfont \\\n>         -yscrollcommand scrolltext -wrap none \\\n>         -xscrollcommand \".bleft.bottom.sbhorizontal set\"\n>      if {$have_tk85} {\n> --\n> 2.9.3\n>\n\nNice that such a simple change results in a huge gain. I think this\nmakes perfect sense.\n\nRegards,\nJake\n"},{"id":"305586","messageId":"xmqqtwbhbql9.fsf@gitster.mtv.corp.google.com","threadId":"44438","inReplyTo":"de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de","subject":"Re: [PATCH 0/3] gitk: memory consumption improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-08T21:37:38Z","receivedAt":"2016-11-08T21:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Hitter <mah@jump-ing.de> writes:\n\n> List, Paul,\n>\n> after searching for a while on why Gitk sometimes consumes\n> exorbitant amounts of memory I found a pair of minor issues and\n> also a big one: the text widget comes with an unlimited undo\n> manager, which is turned on be default. Considering that each line\n> is inserted seperately, this piles up a huuuge undo stack ... for\n> a read-only text widget. Simply turning off this undo manager\n> saves about 95% of memory when viewing large commits (with tens of\n> thousands of diff lines).\n\nYou made me laugh crazy hard while in a waiting room in a clinic\nyesterday with the cover letter; people around gave me a strange\nlook but I couldn't help.\n\nThis is a single liner with the largest gain in the history of this\nproject.  Very well spotted.\n\nAre all semi-modern Tcl/Tk in service have this -undo thing so that\nwe can pass unconditionally to the text widget like the patch does?\n\nIf we had to do this conditionally, that robs the fun of \"just\nadding 8 bytes to the source to reduce 1+GB memory consumption\", but\neven if we had to go conditional, this is a great find.\n\nWell done.\n"},{"id":"305627","messageId":"8eac2a5b-071f-6d17-4d81-0744db16910d@jump-ing.de","threadId":"44438","inReplyTo":"xmqqtwbhbql9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] gitk: memory consumption improvements","fromName":"Markus Hitter","fromEmail":"mah@jump-ing.de","sentAt":"2016-11-09T12:39:34Z","receivedAt":"2016-11-09T12:39:43Z","isPatch":true,"sender":{"key":"mah@jump-ing.de","avatar":"https://avatars.githubusercontent.com/u/318581?v=4"},"body":"Am 08.11.2016 um 22:37 schrieb Junio C Hamano:\n> Are all semi-modern Tcl/Tk in service have this -undo thing so that\n> we can pass unconditionally to the text widget like the patch does?\n\nGood point. As far as my research goes, this flag was introduced in Nov. 2001:\n\nhttp://core.tcl.tk/tk/info/5265df93d207cec0\n\n\nTo defend Gitk developers, the Tk guys apparently change their mind on the default value from time to time. Official documentation says nothing about a default, the proposal from 2001 talks about -undo 0 as default and there are recent commits changing this default:\n\nhttp://core.tcl.tk/tk/info/549d2f56757408f3\n\n\nMarkus\n\n-- \n- - - - - - - - - - - - - - - - - - -\nDipl. Ing. (FH) Markus Hitter\nhttp://www.jump-ing.de/\n"},{"id":"305654","messageId":"xmqqinrw8dcj.fsf@gitster.mtv.corp.google.com","threadId":"44438","inReplyTo":"8eac2a5b-071f-6d17-4d81-0744db16910d@jump-ing.de","subject":"Re: [PATCH 0/3] gitk: memory consumption improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-09T23:04:12Z","receivedAt":"2016-11-09T23:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Hitter <mah@jump-ing.de> writes:\n\n> Am 08.11.2016 um 22:37 schrieb Junio C Hamano:\n>> Are all semi-modern Tcl/Tk in service have this -undo thing so that\n>> we can pass unconditionally to the text widget like the patch does?\n>\n> Good point. As far as my research goes, this flag was introduced in Nov. 2001:\n>\n> http://core.tcl.tk/tk/info/5265df93d207cec0\n\nSounds safe enough then ;-)\n\nThanks for an additional research.\n"},{"id":"307496","messageId":"20161212095119.GF20934@fergus.ozlabs.ibm.com","threadId":"44438","inReplyTo":"de7cd593-0c10-4e93-1681-7e123504f5d5@jump-ing.de","subject":"Re: [PATCH 0/3] gitk: memory consumption improvements","fromName":"Paul Mackerras","fromEmail":"paulus@ozlabs.org","sentAt":"2016-12-12T09:51:19Z","receivedAt":"2016-12-12T09:51:51Z","isPatch":true,"sender":{"key":"paulus@ozlabs.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Mon, Nov 07, 2016 at 07:54:28PM +0100, Markus Hitter wrote:\n> \n> List, Paul,\n> \n> after searching for a while on why Gitk sometimes consumes exorbitant amounts of memory I found a pair of minor issues and also a big one: the text widget comes with an unlimited undo manager, which is turned on be default. Considering that each line is inserted seperately, this piles up a huuuge undo stack ... for a read-only text widget. Simply turning off this undo manager saves about 95% of memory when viewing large commits (with tens of thousands of diff lines).\n> \n> 3 patches are about to follow:\n> \n>  - turn off the undo manager,\n> \n>  - forget already closed file descriptors and\n> \n>  - forget the 'commitinfo' array on a reload to enforce reloading it.\n> \n> I hope this finds you appreciation.\n\nThanks for the good work in tracking this down and making the patches.\nI have applied the series.  Apologies for slow response (life has been\nextremely busy for me this year).\n\nPaul.\n"}]}