Summary: When a ManifestEntry is destroyed, we reap the heap-allocated resources it holds. However, this (erroneously) happens when we create a ManifestEntry and add it to the list of children. What we really need is a move constructor from C++11. The workaround is to initialize a blank ManifestEntry, add it to the proper location, and then populate it afterwards.
Test Plan: addChild (in a later diff) no longer tramples over memory it no longer owns.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3772915
Signature: t1:3772915:1472174144:a4baac5cb5f6e01a38042c5c6cd92570c8f8e100
Summary: This gives us a bit of flexibility when we call this method, as references cannot be reassigned.
Test Plan: compiles
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3772844
Signature: t1:3772844:1472174021:565d7f25a27c2fd78ce14b86b5bc499c74d47976
Test Plan: haven't used this yet, but it makes a whole lot of sense.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3770366
Signature: t1:3770366:1472173969:b83e10e27a63b863a3566d2df7cc861417b8208e
Summary:
1. `children()`, which returns the number of children the manifest has.
2. `removeChild()`, which removes the entry that the iterator references.
Test Plan: used in later diffs
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3770363
Signature: t1:3770363:1472173823:737043b4613a669b402dcfd7921ac55278c3ece9
Summary: The strings are not necessarily null-terminated and length needs to be considered.
Test Plan: used in later diff to find a path.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3770358
Signature: t1:3770358:1472173786:9a2f681c90476aafd481c301ff65ac8b199214ec
Summary:
appendbinfromhex appends the binary representation of a 40-byte hex string onto a std::string. This allows us to reuse a std::string rather than to allocate a new one every time.
Also:
1. converted binfromhex to use this method.
2. updated the docblocks to actually reflect reality.
Test Plan: `make local && cd ~/work/fbsource && PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/:~/work/mercurial/facebook-hg-rpms/lz4revlog/ /opt/local/bin/python2.7 ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extensions.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "master~5::master"`
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3770048
Signature: t1:3770048:1472105520:eac79a42360ebfa258519346b68fc4541c2dbb7c
Summary: In most cases, this is pretty straightforward. The only unusual case is `_treemanifest_find`, which does the actual resolution of the root manifest (unlike diff and iter).
Test Plan: make local
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3763324
Signature: t1:3763324:1472173572:3dcda9b318ad818d2f51e8c3472c7770739faafe
Summary: This more accurately describes its purpose.
Test Plan: simple refactor, so thus just make local
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3763323
Signature: t1:3763323:1472104318:b48246777db0527c7022066438c211f54c88703e
Summary: It makes a destructor possible.
Test Plan: make local
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3763322
Signature: t1:3763322:1472104242:e76ef1943c2082ddecff872c6472de4706505922
Summary: It's a fixed reference that we use, so no need for a pointer.
Test Plan: `PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/:~/work/mercurial/facebook-hg-rpms/lz4revlog/ /opt/local/bin/python2.7 ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extensions.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "master~5::master"`
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3753463
Signature: t1:3753463:1471905506:802315db9d2aa34363c7b0bccaefcbcda21b1a1e
Summary: Since we're no longer returning a struct, we no longer need to return the value through a pointer argument.
Test Plan: `PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/:~/work/mercurial/facebook-hg-rpms/lz4revlog/ /opt/local/bin/python2.7 ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extensions.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "master~5::master"`
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3752376
Tasks: 12818084
Signature: t1:3752376:1471891162:e7cc159d2077d77d234902e4598b9661f15840c0
Summary: This allows us to modify the ManifestEntry stored in memory. This also requires us to remove the const qualifier in a number of places.
Test Plan: `PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/:~/work/mercurial/facebook-hg-rpms/lz4revlog/ /opt/local/bin/python2.7 ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extensions.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "master~5::master"`
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3752366
Tasks: 12818084
Signature: t1:3752366:1471891101:1d42a04d85b7e8db34644dc8fbf1bb3481fbb7bc
Summary: py-treemanifest.cpp will be mostly just python binding stuff.
Test Plan: make local
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3733832
Signature: t1:3733832:1471542792:d6f1130b5c16487f4d0173cedd91857fd3711c1d
Summary: Directly pass in the path + len and the node. Note that the path is now a char* + len, because this allows us to use the path in treemanifest_find directly, rather than to construct a new path.
Test Plan: run existing perftest without crash.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3738280
Signature: t1:3738280:1471890923:c13283f1c61dc020ba1918ee9b25c24dfd2fc19b
Test Plan: used in later diff.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3732102
Signature: t1:3732102:1471905481:000ff5d976c348bac9f993f9c0b56f4b9c8b84f0
Summary:
I like many small files.
There is one place where I'm making a functional change (convert.h) to satisfy angry compilers.
Test Plan: make local.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3732584
Signature: t1:3732584:1471542758:d0b7804753ea4fd39a507090338ae3c5104dc7fa
Summary: For adding new children to Manifests, we need to be able to create new ManifestEntries. Since these ManifestEntries will not be backed by datapack data structures, we need ManifestEntries that have its own memory allocation.
Test Plan: used in later diff.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3732101
Signature: t1:3732101:1471541200:17b60af0977109757610168637a276f5dd999f8b
Summary: D3730823 removes the need for it.
Test Plan: compiles
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mathieubaudet, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3730880
Signature: t1:3730880:1471467917:2b202c5cab1a10fcfe5899670b11bc44740983f7
Summary:
If a manifest has already been loaded for a ManifestEntry, we should cache that entry and reuse it. Two reasons:
1) if someone makes a modification to a tree, we need to persist that.
2) better performance.
Missing in this diff: memory cleanup
Test Plan: compiles
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3725842
Signature: t1:3725842:1471401136:cbf4a987c35ea19ca432059cc15e299f0aa5568b
Summary: This allows us to use ManifestFetcher inside the ManifestEntry class.
Test Plan: compiles
Reviewers: #fastmanifest, durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3725838
Summary: This will allow us to replace portions of manifests with in-memory representations.
Test Plan: existing script doesn't crash.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3725831
Signature: t1:3725831:1471400740:4d9891c01f8567f4ceab76d6bd36e7dc595de4a6
Summary: `parseptr` refers to where the parsing is going next. This simplifies the code and makes ManifestEntry less tied to the original memory allocation.
Test Plan: the existing test i've been running doesn't crash.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3717940
Signature: t1:3717940:1471383988:9b718e137b8ffaf8aad09f78f15791eec57fce6e
Summary: Parse manifests when we construct the Manifest object. This allows us to do manipulations to the Manifest object and not have to deal with a parallel set of data structures.
Test Plan:
`PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/ valgrind ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extensions.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "@~2::@"`
note that I'm running this with Valgrind.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3717936
Signature: t1:3717936:1471383856:012634c1e59f1da9fc1e5a918e7f7d99d30d6992
Summary: It's not necessary for the direction we're going in.
Test Plan: compiles
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3717930
Signature: t1:3717930:1471383901:d8bfddb7ea7fb08d26315cbfce7af65d14662bc8
Summary:
We need manifest objects to be able to stick around in memory, because now they have overrides and all that other good stuff.
This probably introduces a metric ton of memory leaks, but we'll slowly whittle them down.
Test Plan: same script.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3717304
Signature: t1:3717304:1471385439:9269ab248d233a970c6725dbb4ca3eb661f6a96e
Summary: xxx is the C++ class, py_xxx is the python wrapper for it.
Test Plan: make local
Reviewers: #fastmanifest, akushner
Reviewed By: akushner
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3701536
Signature: t1:3701536:1471150787:5f3dfc3a65360233f9ee2ea1757ed4f368f70d62
Summary: py_treemanifest is initialized to 0s. We initialize the `tm` field by explicitly calling the constructor in `treemanifest_init` and we destroy everything by explicitly calling the destructor in `treemanifest_dealloc`
Test Plan: `PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/:~/work/mercurial/facebook-hg-rpms/lz4revlog/ /opt/local/bin/python2.7 ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extensions.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "master~5::master"` in fbsource
Reviewers: #fastmanifest
Subscribers: mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3730823
Summary: If the flag is not present, then `flag` field is set to NULL. In that case, the current code will segfault. Now we will assign `\0` to `*resultflag`.
Test Plan:
run `PYTHONPATH=~/work/mercurial/facebook-hg-rpms/remotefilelog:~/work/mercurial/facebook-hg-rpms/fb-hgext/:~/work/mercurial/facebook-hg-rpms/remotenames/ ~/work/mercurial/facebook-hg-rpms/hg-crew/hg --config extens
.perftest=~/work/mercurial/facebook-hg-rpms/remotefilelog/tests/perftest.py testtree --kind flat,ctree,fast --test fulliter,diff,find --build "@~2::@"`
without crashing.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3700352
Summary: This is the first step in disentangling the C++ code from the python interface.
Test Plan: `PYTHONPATH=~/remotefilelog/build/lib.linux-x86_64-2.6/ python ~/hg/hg --config extensions.remotefilelog=~/remotefilelog/remotefilelog --config extensions.perftest=~/remotefilelog/tests/perftest.py testtree --kind flat,ctree --test fulliter,diff,find --build "master~5000::master"`
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3700627
Signature: t1:3700627:1471382403:d8dc6dc5c295ec55878ca020b91fc0b30d930ce8
Summary:
clang has a bug where the fully qualified class name cannot be used to invoke the destructor https://llvm.org/bugs/show_bug.cgi?id=12350
This is one of the suggested workarounds.
Test Plan: compiles on darwin
Reviewers: #fastmanifest, lcharignon
Reviewed By: lcharignon
Subscribers: mathieubaudet, mitrandir
Differential Revision: https://phabricator.intern.facebook.com/D3692812
Signature: t1:3692812:1470852812:09ef7de2a322034a01f5569b574c47fcc6f0c8d7
This is a large refactor of the original code base. I apologize for it not being
broken up into multiple commits.
This adds the Manifest class (that represents a single Manifest directory
entry), the ManifestFetcher class (which allows fetching children manifests),
and the InMemoryManifest class (that represents an uncommitted Manifest).
The combination of these classes does a few things:
1. It removes most of the references to PythonObj from the primary algorithms.
So in the future we could use this code from other C++ code without relying on
Python.
2. It refactors the manifest access in such a way that we allow manifests to be
stored in either python strings or in memory. This opens the doors to
implementing manifest editting apis.
Summary:
Now that we have a wrapper around python objects, let's add a getattr() function
to easily retrieve attributes off the object.
Test Plan: Ran the perf test
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3674366
Summary:
This patch introduces a PythonObj class which implements the copy-constructor,
destructor, and assignment operator in a way that will manage the ref count
automatically. If we move to C++ 11 we could also implement the move constructor
and move assignment operators to make this even more efficient.
The current implementation allows implicitly converting to and from PyObject*,
which may be questionable design wise, but makes switching to and using this
class much cleaner since we can do things like `PythonObj foo = PyObject_New()`
and `PyObject_DoStuff(myPythonObj)`.
Test Plan: Ran the perf test. It succeeded, and I saw no effect on perf.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3674311
Summary:
Instead of returning NULL and propagating it up the call stack, let's throw
pyexception (which assumes the python error string has already been set) and the
top of the stack can just return NULL.
Test Plan: Ran my perf test suite
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3673804
Summary:
This implements treemanifest.diff(). It takes two manifests and iterates over
them to produce a python dictionary containing the differences.
I'm not proud of this. Just putting it up for review for completeness since it
completes the find, diff, iter trifecta. I need to refactor it to remove some of
the duplication before it gets accepted.
Test Plan:
Ran it as part of a perf test suite using diffs across various
distances. It takes 250ms to diff across 5000 commits, and 900ms to diff across
50,000 commits.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3646003
Summary:
By making this a class we can encapsulate common operations like parsing and
directory checking. The later diff that implements treemanifest_diff uses this
a lot.
Test Plan: Ran my perf tests for find and iter (and for the later patch diff).
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3673228
Summary:
The varargs style Py_BuildValue functions have no idea what type the incoming
arguments are, so it passed the hard coded ints as 32 bit instead of 64bit.
Let's explicitly cast every number being passed to that function as Py_ssize_t
instead.
Test Plan: We were seeing segfaults without this change. Now we don't.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3673217
Summary:
This implements treemanifest.find(), which takes a filename and returns the node
and flag for it, if it exists.
This isn't the prettiest function I've ever written. I need to think about how
to refactor this to unify the various traversal algorithms that are used in
treemanifest.
Test Plan:
Ran it as part of a perf test suite. It's basically 0 milliseconds in
every case.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3645967
Summary:
This implements the basic __iter__ logic. It returns an iterator that returns
every file path in the manifest.
Test Plan:
Ran a separate perf test suite to verify the performance of this. It
can iterate over 1 million files in about 550 milliseconds, assuming a fast
store.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3645890
Summary:
This defines the basic type for representing an iteration over all the keys in
the treemanifest. The next patch fill add the logic that constructs and mutates
this type as it iterates.
Test Plan: I ran a perf test in a future patch that executes all of this code.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3645768
Summary:
This adds getdata and binfromhex functions. These are common functions that will
be used throughout the implementation of the ctreemanifest.
Test Plan: Ran a perf test suit on the code in a later diff.
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3644960
Summary:
This is the basic definition of the c treemanifest type. Future patches will add
functions to this type.
Test Plan: Tested by running a perf suite on a later version of this series
Reviewers: #fastmanifest
Differential Revision: https://phabricator.intern.facebook.com/D3644935
