{"thread":{"id":"42044","subject":"fast-import's gfi_unpack_entry causes too many munmap/mmap cycles","startedAt":"2016-04-16T09:18:39Z","lastAt":"2016-04-17T01:13:48Z","messageCount":5,"participants":["Mike Hommey"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"283640","messageId":"20160416091839.GA12764@glandium.org","threadId":"42044","inReplyTo":null,"subject":"fast-import's gfi_unpack_entry causes too many munmap/mmap cycles","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-16T09:18:39Z","receivedAt":"2016-04-16T09:18:39Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nI think I've mentioned this issue in the past, and fixed one case of\nmunmap/mmap cycle in fast-import. I've found more cases where this can\nhappen, and while on Linux, it's not a problem, on OSX, it leads to\ncatastrophic performance, when the import is massive.\n\nOne part of the problem is that gfi_unpack_entry just assumes that as\nlong as the pack was touched since last access, it _has_ to close all\nwindows and remap the pack. Afaict, when the requested entry is in the\nrange of what was previously mapped, there is no reason to do this, and\nthe following patch would help:\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 75f7748..91fa2a1 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1294,7 +1294,8 @@ static void *gfi_unpack_entry(\n {\n        enum object_type type;\n        struct packed_git *p = all_packs[oe->pack_id];\n-       if (p == pack_data && p->pack_size < (pack_size + 20)) {\n+       if (p == pack_data && oe->idx.offset >= (p->pack_size - 20) &&\n+           p->pack_size < (pack_size + 20)) {\n                /* The object is stored in the packfile we are writing to\n                 * and we have modified it since the last time we scanned\n                 * back to read a previously written object.  If an old\n\nNow, the problem is that this doesn't help with use cases like the\nfollowing:\n- create commit A.\n- get its tree with `ls :mark \"\"`\n\nIn that case, since the tree was (very likely) created by committing,\nthe entry is located beyond the open window for the pack.\n\nThere is some kind of work around for that particular use case: get the\ntree *during* the commit, with `ls \"\"` after all the\nfilemodify/filedelete entries, and before the newline that finishes the\ncommit. Unfortunately, in that case, store_tree does its job twice (once\nfor the ls, and once when committing for real), and that leads to\nsignificant overhead.\n\nAnd even if I am okay with that overhead, I still hit the problem again\nwhen using that tree later with `M 040000 $sha1 ` in the next commit,\nbecause that does a load_tree() for the tree sha1, after a commit having\noccurred, which touched the pack, bringing me back to square one (the\nsad part being that it does so while the tree_content for the tree in\nquestion is already in memory).\n\nNow, while each individual case could be improved to avoid\ngfi_unpack_entry, it seems to me it would be better to make\ngfi_unpack_entry better somehow. I wonder if there's not a better way\nthan having it close and reopen windows all the time for recent objects.\nCan't the pack be pre-allocated and mapped by window-size fragments, for\nexample? (Would that maybe not work properly on all platforms?)\n\nMike\n\nPS: And if I could find a work around that does work with existing\nreleases of git, that would be great. The root of all my pain is that I\nhave tree deltas between consecutive commits, when the commits are not\nnecessarily parent/child. e.g. for a commit history like\n\nA --- B\n \\\n  \\-- C\n\nI have:\n- diff between null-tree and A\n- diff between A and B\n- diff between B and C\n\nWhich means for C, I *need* to create a commit without a `from`, and\ninitialize its tree with `M 040000 sha-1-of-B's-tree `.\n"},{"id":"283641","messageId":"20160416093146.GA15853@glandium.org","threadId":"42044","inReplyTo":"20160416091839.GA12764@glandium.org","subject":"Re: fast-import's gfi_unpack_entry causes too many munmap/mmap cycles","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-16T09:31:46Z","receivedAt":"2016-04-16T09:31:46Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Apr 16, 2016 at 06:18:39PM +0900, Mike Hommey wrote:\n> Now, while each individual case could be improved to avoid\n> gfi_unpack_entry, it seems to me it would be better to make\n> gfi_unpack_entry better somehow.\n\nCome to think of it, there are cases that might still be worth fixing.\nLike avoiding load_tree() altogether when doing `M 040000 $sha1 ` when\nthere is no other filemodify/filedelete following.\n\nMike\n"},{"id":"283643","messageId":"20160416110403.GA19197@glandium.org","threadId":"42044","inReplyTo":"20160416091839.GA12764@glandium.org","subject":"Re: fast-import's gfi_unpack_entry causes too many munmap/mmap cycles","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-16T11:04:03Z","receivedAt":"2016-04-16T11:04:03Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Apr 16, 2016 at 06:18:39PM +0900, Mike Hommey wrote:\n> And even if I am okay with that overhead, I still hit the problem again\n> when using that tree later with `M 040000 $sha1 ` in the next commit,\n> because that does a load_tree() for the tree sha1, after a commit having\n> occurred, which touched the pack, bringing me back to square one (the\n> sad part being that it does so while the tree_content for the tree in\n> question is already in memory).\n\nSo, interestingly, the load_tree() doesn't happen when using `from\n0000000000000000000000000000000000000000\\nmerge :mark` for the commit.\n\nThat is, with:\n  commit refs/foo\n  committer <foo@foo> 0 +0\n  data 0\n\n  M 040000 bff3a42ecae0e7c8f707d328f9432c1ffe9644b0 \"\"\n\nload_tree is called. And with:\n  commit refs/foo\n  committer <foo@foo> 0 +0\n  data 0\n\n  from 0000000000000000000000000000000000000000\n  merge :mark_of_parent\n  M 040000 bff3a42ecae0e7c8f707d328f9432c1ffe9644b0 \"\"\n\nit's not called.\n\nSo I think I got myself a workaround...\n\n> A --- B\n>  \\\n>   \\-- C\n> \n> I have:\n> - diff between null-tree and A\n> - diff between A and B\n> - diff between B and C\n\nI should be able to do:\n\n- start the commit command for A\n- before finishing it, `ls \"\"`\n- then apply the diff for B and `ls \"\"`\n- then apply the diff for C and `ls \"\"`\n- then `deleteall`\n- then `M 040000 sha1_from_first_ls ` and finally finish A\n- create the commit for B with `from\n  0000000000000000000000000000000000000000\\nmerge :mark` and `M 040000\n  sha1_from_second_ls`\n- likewise for C\n\n... and avoid gfi_unpack_entry.\n\nMike\n"},{"id":"283658","messageId":"20160417005443.GA15847@glandium.org","threadId":"42044","inReplyTo":"20160416110403.GA19197@glandium.org","subject":"Re: fast-import's gfi_unpack_entry causes too many munmap/mmap cycles","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-17T00:54:43Z","receivedAt":"2016-04-17T00:54:43Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sat, Apr 16, 2016 at 08:04:03PM +0900, Mike Hommey wrote:\n> So I think I got myself a workaround...\n> \n> > A --- B\n> >  \\\n> >   \\-- C\n> > \n> > I have:\n> > - diff between null-tree and A\n> > - diff between A and B\n> > - diff between B and C\n> \n> I should be able to do:\n> \n> - start the commit command for A\n> - before finishing it, `ls \"\"`\n> - then apply the diff for B and `ls \"\"`\n> - then apply the diff for C and `ls \"\"`\n> - then `deleteall`\n> - then `M 040000 sha1_from_first_ls ` and finally finish A\n> - create the commit for B with `from\n>   0000000000000000000000000000000000000000\\nmerge :mark` and `M 040000\n>   sha1_from_second_ls`\n> - likewise for C\n> \n> ... and avoid gfi_unpack_entry.\n\nAnd it works... as an avoidance of gfi_unpack_entry... but it has its\nown problem: somehow the store_tree() that happens for each of those\n`ls \"\"` commands is storing *all* trees. Even the ones that haven't\nchanged. In terms of a minimalistic fast-import script:\n\nWith:\n  commit refs/FOO\n  committer <foo@foo> 0 +0\n  data 0\n\n  M 644 inline a/a\n  data 1\n  a\n\n  commit refs/FOO\n  committer <foo@foo> 0 +0\n  data 0\n\n  M 644 inline b/b\n  data 1\n  b\n\nstore_tree is called for:\n- b39954843ff6e09ec3aa2b942938c30c6bd1629e\n- 2c3b59f77afa6fea6c1a380eeb0cb1eb292515b5\n- 51e58bf6ce558dd384bbf9d493f9a376f3bcb089\n- a97dda9f3a819113b3b239b9a62edece27136080\n\nWith:\n  commit refs/FOO\n  committer <foo@foo> 0 +0\n  data 0\n\n  M 644 inline a/a\n  data 1\n  a\n  ls \"\"\n  M 644 inline b/b\n  data 1\n  b\n\nstore_tree is called for:\n- b39954843ff6e09ec3aa2b942938c30c6bd1629e\n- 2c3b59f77afa6fea6c1a380eeb0cb1eb292515b5\n- b39954843ff6e09ec3aa2b942938c30c6bd1629e\n- 51e58bf6ce558dd384bbf9d493f9a376f3bcb089\n- a97dda9f3a819113b3b239b9a62edece27136080\n\nNote how b39954843ff6e09ec3aa2b942938c30c6bd1629e is being stored twice\n(it's the tree for a/).\n\nSo in the scenario I'm testing, which has many more trees, I'm trading\n29k gfi_unpack_entry calls and 230k store_tree calls for 1.96M\nstore_tree calls. On even larger trees, I'm not sure that wouldn't make\nthings even worse than they already are with gfi_unpack_entry.\n\nMike\n"},{"id":"283659","messageId":"20160417011348.GA17856@glandium.org","threadId":"42044","inReplyTo":"20160417005443.GA15847@glandium.org","subject":"Re: fast-import's gfi_unpack_entry causes too many munmap/mmap cycles","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-04-17T01:13:48Z","receivedAt":"2016-04-17T01:13:48Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Sun, Apr 17, 2016 at 09:54:43AM +0900, Mike Hommey wrote:\n> On Sat, Apr 16, 2016 at 08:04:03PM +0900, Mike Hommey wrote:\n> > So I think I got myself a workaround...\n> > \n> > > A --- B\n> > >  \\\n> > >   \\-- C\n> > > \n> > > I have:\n> > > - diff between null-tree and A\n> > > - diff between A and B\n> > > - diff between B and C\n> > \n> > I should be able to do:\n> > \n> > - start the commit command for A\n> > - before finishing it, `ls \"\"`\n> > - then apply the diff for B and `ls \"\"`\n> > - then apply the diff for C and `ls \"\"`\n> > - then `deleteall`\n> > - then `M 040000 sha1_from_first_ls ` and finally finish A\n> > - create the commit for B with `from\n> >   0000000000000000000000000000000000000000\\nmerge :mark` and `M 040000\n> >   sha1_from_second_ls`\n> > - likewise for C\n> > \n> > ... and avoid gfi_unpack_entry.\n> \n> And it works... as an avoidance of gfi_unpack_entry... but it has its\n> own problem: somehow the store_tree() that happens for each of those\n> `ls \"\"` commands is storing *all* trees. Even the ones that haven't\n> changed. In terms of a minimalistic fast-import script:\n> \n> With:\n>   commit refs/FOO\n>   committer <foo@foo> 0 +0\n>   data 0\n> \n>   M 644 inline a/a\n>   data 1\n>   a\n> \n>   commit refs/FOO\n>   committer <foo@foo> 0 +0\n>   data 0\n> \n>   M 644 inline b/b\n>   data 1\n>   b\n> \n> store_tree is called for:\n> - b39954843ff6e09ec3aa2b942938c30c6bd1629e\n> - 2c3b59f77afa6fea6c1a380eeb0cb1eb292515b5\n> - 51e58bf6ce558dd384bbf9d493f9a376f3bcb089\n> - a97dda9f3a819113b3b239b9a62edece27136080\n> \n> With:\n>   commit refs/FOO\n>   committer <foo@foo> 0 +0\n>   data 0\n> \n>   M 644 inline a/a\n>   data 1\n>   a\n>   ls \"\"\n>   M 644 inline b/b\n>   data 1\n>   b\n> \n> store_tree is called for:\n> - b39954843ff6e09ec3aa2b942938c30c6bd1629e\n> - 2c3b59f77afa6fea6c1a380eeb0cb1eb292515b5\n> - b39954843ff6e09ec3aa2b942938c30c6bd1629e\n> - 51e58bf6ce558dd384bbf9d493f9a376f3bcb089\n> - a97dda9f3a819113b3b239b9a62edece27136080\n> \n> Note how b39954843ff6e09ec3aa2b942938c30c6bd1629e is being stored twice\n> (it's the tree for a/).\n\nAnd that happens because tree_content_get returns a duplicate of the\ntree, so when parse_ls does a store_tree, it does it on a duplicate, not\non the tree itself. So all the work done by store_tree through parse_ls\nis thrown away.\n\nMike\n"}]}