Summary:
Upstream converted all array.array usages to bytearray, and in doing so they
also changed mdiff.textdiff(array1, array2) to be
mdiff.textdiff(util.buffer(array1), util.buffer(array2)), which we did not do in
our internal implementation.
This fixes that.
Test Plan: Adds a test
Reviewers: #mercurial, quark, simonfar
Reviewed By: quark, simonfar
Subscribers: quark, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4724350
Signature: t1:4724350:1489697253:8f63350bfc30b3079bf38410054b4763942a82f6
Summary:
Upstream has added a new match argument to manifest.diff() and removed the
existing manifest.matches() function, so we need to update our internal usage.
A separate diff will update treemanifest to support the new diff() api.
Test Plan:
Ran the tests, some still fail because of the upstream changes, but
future patches fix those.
Reviewers: #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D4677002
Summary:
Previously we only wrapped manifestlog.__getitem__ because fastmanifest didn't
support the concept of directory manifests. Unfortunately manifestlog.get() is
used in some flat manifest cases too, like in changegroup.py where it just
passes '' as the directory but still uses .get().
This bug has been present since the manifestlog refactor, but only became
exposed now that we've improved our caching and hold on to manifest ctx's for
longer.
This fix is to just wrap manifestlog.get() too. Fixing this exposed some other
inconsistencies with our fastmanifest api.
Test Plan:
shelve now passes in a repo that was hitting an exception because of
this before. I'm still trying to think of how to test this in a test. I've not
been able to repro it in my own repo yet.
Reviewers: #mercurial, quark
Reviewed By: quark
Differential Revision: https://phabricator.intern.facebook.com/D4540677
Tasks: 15991119
Signature: t1:4540677:1486690231:07a95b402913d7dcc31633341e235ee1eb0163bc
Summary:
Previously treemanifest and fastmanifest were largely unaware of each other. If
the fastmanifest was available, we'd use that. If not, we'd try tree. Then we'd
fall back to flat. When comparing two manifests, this could cause problems since
if one manifest was fast and one was tree, we'd have to fall all the way back to
flat manifests to compare them.
This patch adds the ability to build a treemanifest from a fastmanifest for
inmemory manifests that were copied from a commit that has both a fastmanifest
and a treemanifest. We do it by diff'ing the inmemory fastmanifest with the
commit fastmanifest, then applying that diff to the commit's treemanifest. Then
we can compare that tree with the other tree.
This is particularly useful when doing things like 'hg up master' after a pull.
The new master commit probably has a treemanifest, but doesn't yet have a
fastmanifest, while the commit the user is on probably has both. Now we can do
that hg up without having to parse a full manifest.
Once treemanifest has been enabled, every commit (both pulled and user created)
should have treemanifests. Therefore this case should never happen when
comparing two normal commits. So in theory, the only way to hit this case is
when Mercurial does a manifest.copy() on a manifest that has a fastmanifest
(like when reading a workingctx which must copy the commit manifest and apply
working copy changes), since the copy will choose to copy the fastmanifest and
leave the treemanifest behind. This patch addresses that case, so I think there
shouldn't be other cases.
Test Plan: Adds a test that covers this case.
Reviewers: #mercurial, rmcelroy
Reviewed By: rmcelroy
Subscribers: rmcelroy, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D4402604
Signature: t1:4402604:1484216866:1c075f314026aaf608095b4aca37c9a4277ababa
Previously we attached the original node to the copy of the manifest we made.
This is almost always incorrect. In the main case, we make a copy of the p1
manifest during commit and then edit that manifest to form the new manifest. By
storing the node (and never invalidating it), there's the opportunity for the
code to accidentally load the old p1 manifest instead of the in memory one.
The fix is to not pass node to the copy. This affects the tests because some
tests relied on the copied manifest being able to access the original tree/cache
later. It just happened to work because we never modified the manifest before
performing those loads.
This is similar to the patch to matches() that made the match result not have
node either.
This patch also cleans up the fastdelta logic that attempted to read the
_treemanifest directly, instead of going through the normal read path. This is
what actually caught the bug, since it read the treemanifest from disk, while
the in memory cachedmanifest had changes.
Summary:
This fixes a crash that `hybridmanifestctx` does not have `readdelta`
after upstream's b404425704fa. `readdelta` should be fast for vanilla
manifest so just forward the call there.
Test Plan: Modify a file that `.` touched, run `hg amend` and it won't crash.
Reviewers: ttung, #sourcecontrol, durham
Reviewed By: durham
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3948989
Signature: t1:3948989:1475198696:d8ae194500fc093c03cc115f4613a9b8c8bfbff2
Summary: `prune` is essentially `makeroomfor(0, set())`.
Test Plan: pass existing unit tests. output is slightly different, but that's just because we no longer output that debugging line.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3547069
Signature: t1:3547069:1468352305:6185132648f871e26d3cfd449c059523b5eb6d6b
Summary:
Previously, depending on the code path, the limit specified would not actually take effect. For instance, if we came in from debugmanifestcache, and attempted to populated the cache, we would use `systemawarecachelimit` when filling the cache, and the fixedsize limit specified by the user when pruning.
With this change, we unify the all the cache limit decisions to `fastmanifestcache`. If the user actually overrides the limit, we set the limit in `fastmanifestcache` and let that make the decisions.
We also change the definitions of limit in `hg debugcachemanifest` to:
1) >0 => it's the limit.
2) =0 => use systemawarecachelimit
3) <0 => no limit!
Test Plan: pass existing unit tests. there's a small change in the test output, because we always evaluate the limit now, plus we remove the test for limit=0, since it means something different now.
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: trunkagent, mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3544997
Signature: t1:3544997:1468281604:8f78f00ebf2afd8f3f1fbefbd82316b97cc4b193
Summary:
We need a fastmanifest object in order to size it. Once we know its size, we can make room in the cache.
This slightly affects one of the tests, as we request the manifest text earlier than we previously did.
Depends on D3537904
Test Plan: used in later diff.
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters, quark
Differential Revision: https://phabricator.intern.facebook.com/D3538991
Signature: t1:3538991:1468280648:41c65d91529babe0559eac7b75509481adf2765f
Summary:
Before this patch we were doing two mistakes in the ratio computation.
- We were not recording global cache hit or miss, it was always a hit because
a function is truthy is python
- We were not deduping cache miss for the same manifest multiple times
This patch fixes these two mistakes.
It also changes the logging of cache hit and miss to include the name of the
operation that triggered the cache hit or miss (diff or filesnotin).
Test Plan: Test output changed and commented
Reviewers: durham, ttung
Differential Revision: https://phabricator.intern.facebook.com/D3484658
Summary:
Before this patch we were using config instead of configbool for reading
the debugmetrics config causing "False" to be evaluated as a truthy value
for the config. This patch fixes the issue and sets the config to false for
some of the tests to reduce the noise of the output.
Test Plan: Tests pass
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3524501
Summary: D3505464 modified the kwargs, but the test output was not updated.
Test Plan: passed tests.
Reviewers: lcharignon, #mercurial, rmcelroy
Reviewed By: rmcelroy
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3506179
Signature: t1:3506179:1467353060:7c7cf9fd51257f5f5aa037a779618cf6512c1dd0
Summary:
Since metrics doesn't send a string, let's just stub out the call.
The alternative is to put a '' in every `recordsample()` call.
Test Plan: pass existing tests.
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3479138
Tasks: 11900487
Signature: t1:3479138:1466787619:910917093cf0361ed181af5597ab702b936f8d1e
Summary: It's actually most relevant to least relevant. Add a docblock.
Test Plan: pass unit tests
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3478616
Signature: t1:3478616:1466787566:b38eceddf91c3e88341f9173e07244ac33ab6345
Summary:
Everything will be synchronous within the process. We will do background work in a separate process.
Depends on D3468827
Test Plan: updated tests to remove the one background task.
Reviewers: durham, lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3468828
Tasks: 11683504
Signature: t1:3468828:1466694717:556968a10717c3dd7ce403593cc6a1e97067633f
Summary:
This patch fixes various things around the sampling extension to
match what our wrapper expects to see. See detail in the test.
Test Plan:
lcharignon@XXX fbsource cat ~/.hgrc
...
[extensions]
sampling=
[sampling]
key.fastmanifest-cachehitratio=table_blah
key.fastmanifest-trigger=table_blah
filepath=/dev/shm/samppath
lcharignon@XXX fbsource hhg book foo
lcharignon@XXX fbsource cat /dev/shm/samppath
{"category": "table_blah", "data": {"source": "bookmark", "metrics_type":
"fastmanifest-trigger"}}\0{"category": "table_blah", "data": {"ratio": -1, "metrics_type":
"fastmanifest-cachehitratio"}}\0
Reviewers: ttung, durham
Differential Revision: https://phabricator.intern.facebook.com/D3466719
Summary:
Before this patch, if we decided to remove entries A,B,C where A is
more recent than B, which in turn is more recent than C. We were removing
in order A,B and C. This patch changes the order to C,B and A and will make
the next patches simpler.
Test Plan: Tests are updated accordingly
Reviewers: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3427795
Summary:
When refreshing cache entries, it is useful to know what delay we
introduce. This patch adds a debug line showing that information.
Test Plan: tests are updated accordingly
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3427794
Summary:
Before this patch, it wasn't clear from hg debugcachemanifest --list
which manifest were older and newer or what rev were actually cached. This
patch improves the output of hg debugcachemanifest --list --debug to show
this information.
Test Plan: add debug output to the test
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3427791
Summary:
Before this patch we were returning the cache entries alphabetically.
This patch changes the behavior to return the entries sorted by date and
alphabetically. This will be used to simplify the code for pruning entries
in furter patches.
Test Plan: test output changes
Reviewers: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3427789
Summary: Add code to log metrics for fastmanifest, also handles aggregate
metrics. We reuse ui.log for logtoprocess. We also refactor diffs and
filesnotin to make it easier to log cache hit and miss ratio.
Test Plan:
Replaced ui.log by ui.status and aggregated the field, check
that I see metrics getting logged.
Reviewers: ttung, durham
Differential Revision: https://phabricator.intern.facebook.com/D3368504
Summary:
We used to have test code to make the test deterministic. Now we
enforce the same constraint in code and that code is not necessary anymore.
We change the test output to match the new, deterministic output.
Test Plan: Ran the tests 80 times and checked that the output is stable
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3387259
Summary:
This will allow us to know what revisions are actually cached vs
revisions that would be cached by a trigger.
Test Plan: Add new tests
Reviewers: ttung, durham
Differential Revision: https://phabricator.intern.facebook.com/D3385955
Summary:
`lrucachedict`'s `get` method is a lower-level API that retrieves a _lrucachenode object, not what we want. the idiomatic construction is:
```
if key in lrucachedictobj:
do_something(lrucachedictobj[key])
```
Test Plan: hg amend with fastmanifest works. wrote a test that failed before this diff, passed after.
Reviewers: #mercurial, lcharignon, rmcelroy
Reviewed By: rmcelroy
Subscribers: rmcelroy, mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3384502
Tasks: 11569523
Signature: t1:3384502:1465004304:8dfee6a4d0b2f6bb39262a310744458838cb0bf3
Summary: dirstate changes are overly broad. we really only want the trigger if we make a new commit.
Test Plan: will update tests after this series of commits, as the tests are not really exercising the code paths we want.
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D3363013
Tasks: 11475606
Signature: t1:3363013:1464465629:8aad5cbb5f357e751a11aab593347327c50314b5
Summary:
This is somewhat insulated against potential future design changes in chg, as we record the last pid to execute `cachemanifestfillandtrim`, and only suppress it if the pid is unchanged.
Notice the unit test no longer does extra calls to populate the cache.
Test Plan: passed unit tests
Reviewers: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3357697
Summary: When --debug is turned on for commands, there's a ton of spew. This makes it easier to spot the parts I care about.
Test Plan: update tests
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3357974
Signature: t1:3357974:1464365624:83edc815109db8533465991413a7c7741f1056a8
Summary:
As discussed it is not relevant as the tests cache so little it should
not make the test non-deterministic.
Test Plan: The test change accordingly since the limit is no longer None
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3353394
Summary: This will limit what we cache in the first place
Test Plan:
No test yet, I will add a test in a diff later checking cache
eviction.
Reviewers: ttung, durham
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3346515
Summary:
If we want to cache revision [300, 350, 320, 380], we would first order the
revisions to be [380, 350, 320, 300] and cache them in that order. Assuming that
only 3 revisions can fit in the cache, we write all revisions and prune the
one that overflew. Afterwards, we change the mtime of all the cached entries
to ensure that for each of the newly cached entries, more recent mtime == more
relevant entry.
Test Plan: Test output changes slightly, as expected
Reviewers: mitrandir, ttung, simonfar
Differential Revision: https://phabricator.intern.facebook.com/D3344125
Summary: Interpret revrange inside the worker process. The reason this is needed is that `triggercacheondirstatechange` happens in the wlock release callback. However, the lock is not released at this point. Interpreting the revrange requires wlock, so then we try to acquire it but we'll never succeed (because we need to finish the wlock release callback). If we do it in the scope of the worker process, it'll try to acquire it, find that the primary process owns it, and wait to acquire it.
Test Plan: run unit tests
Reviewers: durham, lcharignon
Reviewed By: lcharignon
Subscribers: quark, mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3339805
Tasks: 11385141
Signature: t1:3339805:1464209208:ec2424f84570d489aa5c0c629a15b69b6b126c60
Summary: This could be useful to have the number of valid entries in the cache
Test Plan: Test output changes accordingly
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3343488
Summary:
Before this patch with fastmanifest.cacheonchange on, we would cache
manifest on change to dirstate, bookmarks and remotenames. This patch makes
these operation in the background by default. Also it changes the cache strategy
used by default to be the new strategy that adapts based on the system resources.
Test Plan: Tests does not change
Reviewers: ttung, simonfar, quark
Differential Revision: https://phabricator.intern.facebook.com/D3341681
Summary:
I didn't fix the non determinism properly before, this patch fixes
the issue with a more defensive code.
Test Plan: Ran the test 32 times and the result didn't vary
Reviewers: ttung, simonfar, quark
Differential Revision: https://phabricator.intern.facebook.com/D3341032
Summary: If we don't, then a lazily-evaluated set is returned. That lazily-evaluated set will suppress the hidden bit when dirstate changes are involved.
Test Plan: on a dirstate change with a lot of hidden revs, observe that only a small set of revs are cached.
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D3338252
Signature: t1:3338252:1464107195:fd0e342fb4e4eb485057b7fda9679f7ff18f4e7b
Summary:
Before this patch, we were iterating over the cache entries in a non-
deterministic fashion. This patch makes the iteration stable by ordering the
entries by filename.
Test Plan: Changed the tests accordingly
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3336880
Summary:
We order by mtime and then by filename and not just by mtime. Otherwise, we
wrongfully relied on os.listdir to return the same thing on different platforms.
Test Plan: Change the test
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3336874
Summary:
When we start worker process to cache revisions, since we don't use
a lock, it is better to have the workers cache revisions in a random order to
avoid duplicated effort as much as possible.
Test Plan: No test for now, not sure how to test that.
Reviewers: simonfar, quark, ttung
Differential Revision: https://phabricator.intern.facebook.com/D3335918
Summary:
This adds the first, basic implementation of size limitation for
the fastmanifest cache
Test Plan: Add a few tests that cover edge cases
Reviewers: ttung, simonfar, durham
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3326149
Summary:
This patch makes it possible to cache fastmanifest asynchronously. I added
a test an also tested with fbsource:
hg debugcachemanifest --debug --config extensions.fastmanifest=/data/users/lcharignon/facebook-hg-rpms/fb-hgext/fastmanifest.py -R ~/fbsource --all
hg debugcachemanifest --debug --config extensions.fastmanifest=/data/users/lcharignon/facebook-hg-rpms/fb-hgext/fastmanifest.py -R ~/fbsource --list
This is a tricky code, and I think @quark would be best to review it.
Test Plan: Added a new test
Reviewers: quark, durham, ttung
Subscribers: quark
Differential Revision: https://phabricator.intern.facebook.com/D3323540
Summary: This is a refactoring of the _cachemanifest function
Test Plan: test output does not change
Reviewers: ttung, simonfar
Differential Revision: https://phabricator.intern.facebook.com/D3330501
Summary:
We use the manifestnode from the changelog and avoid having to reach the manifest
completely to check if it is already in cache.
This goes with a refactoring of the contains method. Before this patch, the
contains method was misleading because an entry could be contained in the cache
but not show up when you iterate over the cache. This happened because contains
was operating on nodes and iterating showed filesnames (so keys + prefix). To
make it clear that contains operates on nodes, we change its name to containsnode
We also rename key to hexnode to make it clearer that the keys are hexnodes.
Test Plan: Existing tests pass. We also add a new test to show that the fast
pass is hit.
Reviewers: durham, ttung
Differential Revision: https://phabricator.intern.facebook.com/D3325335
Summary: This will be useful for implementing garbage collection later.
Test Plan: Add a new test that exercises the new function
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3322426
Summary:
This patch changes fastmanifest to add automatic caching of relevant
manifest when bookmarks change or if the parent of the working copy changes.
Test Plan: Added a test
Reviewers: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3318775
Summary:
Adding a pruneall method to prune all cache manifest. This will be
useful to test the caching logic without having to run shell commands to remove
manifests.
Test Plan: Add a new test
Reviewers: ttung, durham
Differential Revision: https://phabricator.intern.facebook.com/D3314158
Summary:
* `_manifest()` should attempt to retrieve a fastmanifest if possible.
* `self.incache` represents a tristate indicating True if the fastmanifest is available, False if the fastmanifest is not, and None if we haven't tried to determine its availablility.
* clarified the debug messages a bit
* re-introduce the test removed in D3277498 (since it actually can work now).
Test Plan: pass unit tests!
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3288967
Signature: t1:3288967:1463416544:416f28ecdc5e6a26545f3d4215fc8baa9222af7c
Summary: Flat manifests are cached in memory by manifest.py. Unclear whether there's any reason for us to cache flat manifests on disk.
Test Plan: ran test-fastmanifest.py
Reviewers: durham, lcharignon
Reviewed By: lcharignon
Subscribers: trunkagent, mitrandir, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D3277498
Tasks: 10589051
Signature: t1:3277498:1463416673:74cab287b9045166dc0322a45f8a325f65da5d1f
