Summary:
This adds support for the optional ctreemanifest type. If the module is
available and if `fastmanifest.usetree` is enabled (default: False), we'll use
tree manifests before falling back to flat manifests.
Test Plan: Ran the ctreemanifest perf suite and the fastmanifest test suite
Reviewers: #fastmanifest, ttung
Reviewed By: ttung
Subscribers: ttung, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3754309
Signature: t1:3754309:1471929802:3ccba8ce6160b7a6d20b599626efd5876bd969f6
Summary:
This assert is meant to check that at least one source was provided to the
hybridmanifest. The old version was broken though.
The test requires an update because it attempts to construct a hybridmanifest
with no source.
Test Plan: Ran the tests
Reviewers: #fastmanifest, ttung
Reviewed By: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3760229
Signature: t1:3760229:1472062113:695085444915cb15a93db7f317580dcffc2f115d
Summary:
This adds the `fastmaniest.usecache` config option (default: True) which lets
users disable the use of the fastmanifest cache without disabling the entire
extension. This will be useful in a future patch where we'll use this same
hybrid manifest infrastructure to allow swapping in tree manifests (even on
repos that don't want fast manifest caches).
Also contains a minior fix to an assert that didn't appear to make sense (I
think it should be asserting that at least one data source is provided).
Test Plan: Ran the ctreemanifest perf suite and the fastmanifest test suite
Reviewers: #fastmanifest, ttung
Reviewed By: ttung
Subscribers: ttung, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3754301
Signature: t1:3754301:1471990417:0cde06f271d10c7f0c839e4f644310a4f525f3f6
Summary:
Because we now prune in parallel to priming the cache, we no longer need the prune at the end. The only scenario where we still need to prune is where we never enter the priming loop, i.e., when the revset is empty.
Depends on D3545267, D3544997
Test Plan: since we don't prune in many circumstances any more, the test output is slighty affected. otherwise, the tests pass.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: durham, mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3547219
Signature: t1:3547219:1468373775:e4535e3a66fb362385a23b992749eb7c3fcf7ea1
Summary: vfs.join(None) will return its base. doesn't matter if it's nested or whatever.
Test Plan:
cloned fbjava, then stripped a revision (with evolve off). then added a new revision, and ran `hg incoming`. this resulted in the same stacktrace!
then with the fix run `hg incoming` again. no crash this time.
Reviewers: #fastmanifest, simonfar
Reviewed By: simonfar
Subscribers: mitrandir, simonfar, mjpieters, lcharignon
Differential Revision: https://phabricator.intern.facebook.com/D3598348
Tasks: 12305684
Signature: t1:3598348:1469093045:440cf3d314589f46dd8083901b584d7cfd54a95a
Summary: Sometimes opener.vfs is an _fncachevfs, which has a .vfs member, not a .base member. When you get one of these, go deeper.
Test Plan:
Run testrunner.py flib/intern/sandcastle/vcs/ without this change, see it traceback.
Rerun testrunner.py flib/intern/sandcastle/vcs/ with this change, see it fail in the same way as the current released version.
Reviewers: durham, lcharignon, ttung, rmcelroy
Reviewed By: rmcelroy
Subscribers: rmcelroy, mjpieters, #sourcecontrol
Differential Revision: https://phabricator.intern.facebook.com/D3593097
Tasks: 12305684
Signature: t1:3593097:1469033487:aec4c4a5de6f92a64f4730f03e9780e02f086819
Summary: Rather than have fastmanifestcache as a singleton, we attach it to each opener.
Test Plan: pass existing unit tests.
Reviewers: #fastmanifest, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3553542
Tasks: 12169797
Signature: t1:3553542:1468373683:b0a7fcdf5dbf12046e5b4be9aa0bc5d46e55ce06
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:
It's kind of silly to write the FM cache entry, and then immediately blow away the entry if it doesn't fit in the limit. Instead, just calculate if we have enough space, and raise CacheFullException if we don't.
Depends on D3538944
Test Plan: pass existing tests.
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters, quark
Differential Revision: https://phabricator.intern.facebook.com/D3545446
Signature: t1:3545446:1468283437:7e4167e3b5a13bf3d01fca45a965d7aaa8843166
Summary:
There's two issues with `setwithlimit`:
1) It does not have a docblock, nor an API that fully covers all the possible conditions.
2) It returns None if the cache entry is already present.
This modifies `setwithlimit` to return True if the cache entry ultimately makes it (whether it previously existed or the write was successful), False if the write fails, and raises CacheFullException if the cache is full.
Test Plan: pass existing tests. the already-in-cache case is a race condition that is not practical to reproduce.
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3545407
Signature: t1:3545407:1468283390:64d6cdba7a17cca9bd4465c6e03b9143e4b4cc4b
Summary:
Instead of pruning at the end, we need to prune as we are writing entries.
Depends on D3545060
Test Plan: pass test in D3545060
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3545267
Signature: t1:3545267:1468279898:5332838efa52bb55733d0e6dcf613b27e71e9d3e
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: This allows us to convert a manifest once, and try to repeatedly jam it into the cache (presumably while evicting things).
Test Plan: pass existing tests.
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters, quark
Differential Revision: https://phabricator.intern.facebook.com/D3537904
Signature: t1:3537904:1468268209:6f1746d3cee5071321d830bb91efd591a31a8616
Summary: We don't need this since we loosely lock the cache worker.
Test Plan: pass existing tests.
Reviewers: lcharignon, durham
Reviewed By: durham
Subscribers: mitrandir, mjpieters, quark
Differential Revision: https://phabricator.intern.facebook.com/D3537355
Signature: t1:3537355:1468268248:ebf7de5bf5030b90039823e1111a073d7eacc447
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:
There's no need for the `setup()` to be called in `reposetup()` any more.
Depends on D3510966
Test Plan:
1) pass existing unit tests.
2) kill chg server instances, then run `USE_CHG= hg diff -c . --debug` repeatedly. (USE_CHG tells my hg wrapper to use chg)
Reviewers: quark, lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3511004
Signature: t1:3511004:1467410145:0761c80da77d5a193fbdb08527fcc4aae9f8065e
Summary: Whenever fastmanifest needs an ui object, we pass it a global proxy object. Whenever someone calls `reposetup(..)`, we update the backing object.
Test Plan:
1) pass existing unit tests
2) enable chg, then run `hg diff -c . --debug`. saw fastmanifest debug output in the console.
Reviewers: lcharignon, quark
Reviewed By: lcharignon
Subscribers: trunkagent, mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3510966
Signature: t1:3510966:1467410140:88fe810d03b9a9aa6818f17eb605a70eee931364
Summary:
There are two parts to add logging to blackbox. Logging events
and changing the configuration of blackbox. This diff does the former.
We don't reuse all the metrics event as:
- blackbox does not support wildcard for events like fastmanifest-*
- they are not meant to be human readable, they are for performance logging
We instead log a few new events:
- What triggered a caching operation?
- What command we run to cache the manifest?
- What revisions are to be cached?
- What revisions are actuallt cached?
- Do we overflow the cache?
This will allow us to understand all the cases where caching didn't trigger.
Like in t11877434.
Test Plan:
Test output does not change
Verified manually by changing the blackbox configuration that events get logged:
CHGDISABLE=1 hg book food --config blackbox.track='command, commandfinish, commandexception, exthook, fsmonitor, pythonhook, fastmanifest' --config extensions.fastmanifest=/home/lcharignon/facebook-hg-rpms/fb-hgext/fastmanifest --config extensions.blackbox=/home/lcharignon/facebook-hg-rpms/hg-crew/hgext/blackbox.py
Reviewers: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3504158
Tasks: 11877434
Summary: If the key as always 'ratio', then all the ratios are aliased when we send off aggregate stats. That means we end up sending the last entry, rather than each individual entry.
Test Plan:
along with D3504940
run FB_HG_DIAGS= hg diff -c .:
```
"int": {
"builddate": 1467289392,
"cachehitratio": 100,
"consumed": 489,
"diffcachehitratio": -1,
"elapsed": 280,
"errorcode": 0,
"filesnotincachehitratio": 0,
"time": 1467318481
},
```
Reviewers: lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3505464
Tasks: 12019647
Signature: t1:3505464:1467320144:327e0d306b9afa90ed9fc2d704a8ca02a79f3038
Summary:
This diff removes `self.ui = ui` in metricscollector to address stale ui
issues with chg. Also clean up unnecessary sample merges since there is
only one collector per process.
Test Plan:
Run with `fastmanifest.debugmetrics=1` and confirm see lines like:
[FM-METRICS] Begin metrics
[FM-METRICS] kind: cachehitratio, kwargs: [('ratio', 100.0)]
[FM-METRICS] kind: diffcachehitratio, kwargs: [('ratio', -1)]
[FM-METRICS] kind: filesnotincachehitratio, kwargs: [('ratio', 0.0)]
[FM-METRICS] End metrics
Reviewers: #mercurial, ttung, lcharignon
Reviewed By: lcharignon
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3504442
Signature: t1:3504442:1467311563:5904bd06b210fa8550b2e1f7e85c2133ad7a1a0e
Summary:
chg runs extsetup and uisetup with dummy ui objects. Before this
patch we were relying on ui objects in extsetup and uisetup. After this
patch, we move the logic to reposetup. We do things a little differently
because reposetup can be called multiple times and extsetup and uisetup are
called only once.
This caused one problem for `hg clone` in the test as reposetup is not
called for clone operation. I adapted the test a little bit to cover this case.
Test Plan: Modified tests pass
Reviewers: mitrandir, quark, lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3499176
Signature: t1:3499176:1467244335:a70d718725c1d19ab6b6feb4558a8ffd1a020ab2
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:
Since we no longer need to unlock in only one fork, we can use the `with lock() as xxx:` syntax
Depends on D3468830
Test Plan: pass existing unit tests
Reviewers: durham, lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3468832
Tasks: 11683504
Signature: t1:3468832:1466694908:6e1d3beff673df5b70ece250af72ad9be66545a6
Summary:
Introduce a cacher class, which encapsulates all the logic for actually caching manifests. Triggers can either directly invoke cacher, or invoke `hg cachemanifest`, which then calls cacher.
Depends on D3468828
Test Plan: pass existing unit tests
Reviewers: durham, lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3468830
Tasks: 11683504
Signature: t1:3468830:1466695022:9a470fcb06badd9e90931cd931a945e8039d71ea
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:
We want to be able to dump debugging output for the workers.
Depends on D3468818
Test Plan: used in later diffs.
Reviewers: durham, lcharignon
Reviewed By: lcharignon
Subscribers: mitrandir, mjpieters, #mercurial
Differential Revision: https://phabricator.intern.facebook.com/D3468827
Tasks: 11683504
Signature: t1:3468827:1466695135:bb930ac6e34f3654a7006634c22dc99b3d2e75bf
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:
This patch adds a test that uncovers logic bugs in the cache. It also
adds fixes for these bugs.
Test Plan:
Add a new test and ran existing tests, observed no change for
exisiting test.
Reviewers: durham, ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3450430
Summary:
This diff refactors the fastmanifest cache. Main differences introduced:
- We use python magic methods to access the cache (__getitem__, __contains__, ...)
- The cache is separated in two classes: ondiskcache and fastmanifestcache, the
former handles all the I/O, the latter handles the cache hierarchy and logic.
This allows to better group I/O related operations and make it easier to reason
about the code.
- By default we use systemawarecachelimit when creating a cache instead of no
limit before.
- The code is simpler to understand, the interface is cleaner: we only use
manifest nodes not a mix of filepath, manifestnodes and cachekeys.
Test Plan: test output does not change
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3427797
Summary: This will be useful to refactor the cache
Test Plan:
We change the python test to give a vfs that makes sense, it will
be useful in the next patch when we refactor the cache. Indeed, with the new
cache we use systemawarecachelimit by default instead of no limit
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3427796
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:
Before this patch if the path was not accessible we would fail to
compute the limit.
Test Plan:
We should test systematically all these kind of issues, I am doing
a refactoring that will allow us to do that.
Reviewers: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3424996
Summary:
Before this patch we were stopping execution when the lock couldn't
be taken. This was problematic when running hg command for someone else's repo
(for example to debug problems). This patch allows us to continue the execution
of the command when that happen, except we won't use fastmanifest.
Test Plan: This is reflected in the test output
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3422458
Summary: This could break people using fastmanifest in someone else's repo
Test Plan: tests pass, not sure how we normally test permission stuff
Reviewers: ttung
Subscribers: mjpieters
Differential Revision: https://phabricator.intern.facebook.com/D3420870
Summary:
Before this patch we would have error using fastmanifest on repos you
didn't own.
Test Plan: tests pass
Reviewers: ttung
Differential Revision: https://phabricator.intern.facebook.com/D3420863
