Merge lp:~vila/bzr/948339-config-caching into lp:bzr

[Vincent Ladeuil]

Properly share mutable config sections and save the branch config only during the final unlock

This fix two bugs in the config stack API:

1) the branch config was saved (if changes have been made) for every
   unlock(). This now happens only for the highest level lock.

2) mutable sections weren't properly shared

On top of this proposal I did two experiments to purge the cached config on
unlock():

1) Define a _clear_cached_state() method for BzrBranch adding a
   self.conf_store_unload().

Chaos ensued, more than 130 tests failed. _clear_cached_state() cannot be
used to unload the config store because it's used in specific ways that go
beyond what can be deduced from its name (see last_revision_info() and
_set_revision_history() for examples :-/)

More concretely, _clear_cached_state() is not a general clear-branch-state
method so it can't be used here.

2) calls self.conf_store.unload() just after save_changes() in unlock().

All test passed *but* using -Econfig_stats, we went, for the whole test
suite from config.load : 54487 to config.load : 114282, roughly doubling
branch.conf accesses. This is caused by all the methods decorated with
@needs_read_lock trigerring a config purge which of course goes against the
aim of reducing the IOs.

Wrong idea IMHO.

So unlocked branch objects will keep their config cached specifically to
cover the use case of unlocked branch objects.

Fixing this requires switching to a model where we never use unlocked
branches which in itself is a really big change which may be valuable but
which has so many fallouts that I don't think we want to tackle it for now.

[Martin Packman (gz)]

Summarising from discussion on IRC yesterday,

The lock count change doesn't seem to be directly asserted by test_set_delays_write. Checking now though the test does in fact fail without the fix, as does test_set_from_config_stack_get_from_config in a similar manner, so that's fine.

Flipping the expectedFailure assertions where the behaviour isn't worth fixing is a little odd, though the added reasoning is nice. Ideally, it would live not here but somewhere that people trying to use config and running into problems might actually find.

The added test_mutable_section_shared is nice and easy to follow, so even though I find the list->dict fixes slightly mysterious without working out more about the config details, I'm happy with the change.

review: Approve

[Vincent Ladeuil (vila)]

> Summarising from discussion on IRC yesterday,
>
> The lock count change doesn't seem to be directly asserted by
> test_set_delays_write. Checking now though the test does in fact fail without
> the fix, as does test_set_from_config_stack_get_from_config in a similar
> manner, so that's fine.

Yeah, the more I think about remarks like this (or one I do myself during reviews), the more I believe I should keep more detailed notes while working on the fix and have a way to mention the narrative in the cover letter... You rightly understand how I proceeded, well done Watson ;) Err, Sherlock ? ;-p

>
> Flipping the expectedFailure assertions where the behaviour isn't worth fixing
> is a little odd,

It's more than 'not worth fixing', it's really that trying to maintain compatibility between the two APIs can't be done while still reducing the IOs. The old API *really* relies in many ways on the fact that each and every change is saved to disk. No matter how you use the old API, you will never miss a change: from different objects in the same process, from different processes, everything works. This came with a price: read the config file for each and every option. Read *and* write the config file for each and every change.

The new implementation already dropped some possible optimisations to ease the transition and is safe (AFAIK) to mix with the old one *as long* as one API only is used for a given option.

So dropping the IO reduction for compatibility sake ? I say no. Some migrations will be harder than others but well, they'll have to happen, so the sooner the better.

> though the added reasoning is nice. Ideally, it would live
> not here but somewhere that people trying to use config and running into
> problems might actually find.

Should I refer to these tests in doc/developers/configuration.txt then ?

>
> The added test_mutable_section_shared is nice and easy to follow, so even
> though I find the list->dict fixes slightly mysterious without working out
> more about the config details, I'm happy with the change.

Ok. I'll add a comment to better explain why it's important that MutableSection should stay unique to be truly shared in the __init__ method then, would that have helped ?

[Martin Packman (gz)]

> So dropping the IO reduction for compatibility sake ? I say no. Some
> migrations will be harder than others but well, they'll have to happen, so the
> sooner the better.
>
> > though the added reasoning is nice. Ideally, it would live
> > not here but somewhere that people trying to use config and running into
> > problems might actually find.
>
> Should I refer to these tests in doc/developers/configuration.txt then ?

Not the tests, but something that would have helped Aaron when he was wondering why the things these tests exercise don't work would be good. A general migration thing with warnings about the pitfalls would be nice, currently that doc just lists some some ai changes without further advice.

> Ok. I'll add a comment to better explain why it's important that
> MutableSection should stay unique to be truly shared in the __init__ method
> then, would that have helped ?

Maybe a little, my issue was mostly that I'm not familiar with what the code is doing.

[lp:~vila/bzr/948339-config-caching]

6502. By Vincent Ladeuil on 2012-03-28

Clarify the dict usage to share dirty sections.

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2012-03-14 15:26:01 +0000
+++ bzrlib/branch.py	2012-03-28 15:52:30 +0000
@@ -968,8 +968,8 @@
         This means the next call to revision_history will need to call
         _gen_revision_history.
 
-        This API is semi-public; it only for use by subclasses, all other code
-        should consider it to be private.
+        This API is semi-public; it is only for use by subclasses, all other
+        code should consider it to be private.
         """
         self._revision_history_cache = None
         self._revision_id_to_revno_cache = None
@@ -2495,7 +2495,7 @@
 
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
-        if self.conf_store is not None:
+        if self.control_files._lock_count == 1 and self.conf_store is not None:
             self.conf_store.save_changes()
         try:
             self.control_files.unlock()

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-03-14 09:30:48 +0000
+++ bzrlib/config.py	2012-03-28 15:52:30 +0000
@@ -2956,7 +2956,7 @@
             # Report concurrent updates in an ad-hoc way. This should only
             # occurs when different processes try to update the same option
             # which is not supported (as in: the config framework is not meant
-            # to be used a sharing mechanism).
+            # to be used as a sharing mechanism).
             if expected != reloaded:
                 if actual is _DeletedOption:
                     actual = '<DELETED>'
@@ -2982,8 +2982,9 @@
     mutable_section_class = MutableSection
 
     def __init__(self):
-        # Which sections need to be saved
-        self.dirty_sections = []
+        # Which sections need to be saved (by section id). We use a dict here
+        # so the dirty sections can be shared by multiple callers.
+        self.dirty_sections = {}
 
     def is_loaded(self):
         """Returns True if the Store has been loaded.
@@ -3032,7 +3033,7 @@
         raise NotImplementedError(self.save)
 
     def _need_saving(self):
-        for s in self.dirty_sections:
+        for s in self.dirty_sections.values():
             if s.orig:
                 # At least one dirty section contains a modification
                 return True
@@ -3052,11 +3053,11 @@
         # get_mutable_section() call below.
         self.unload()
         # Apply the changes from the preserved dirty sections
-        for dirty in dirty_sections:
-            clean = self.get_mutable_section(dirty.id)
+        for section_id, dirty in dirty_sections.iteritems():
+            clean = self.get_mutable_section(section_id)
             clean.apply_changes(dirty, self)
         # Everything is clean now
-        self.dirty_sections = []
+        self.dirty_sections = {}
 
     def save_changes(self):
         """Saves the Store to persistent storage if changes occurred.
@@ -3142,7 +3143,7 @@
 
     def unload(self):
         self._config_obj = None
-        self.dirty_sections = []
+        self.dirty_sections = {}
 
     def _load_content(self):
         """Load the config file bytes.
@@ -3196,8 +3197,7 @@
         if not self._need_saving():
             return
         # Preserve the current version
-        current = self._config_obj
-        dirty_sections = list(self.dirty_sections)
+        dirty_sections = dict(self.dirty_sections.items())
         self.apply_changes(dirty_sections)
         # Save to the persistent storage
         self.save()
@@ -3238,13 +3238,16 @@
         except errors.NoSuchFile:
             # The file doesn't exist, let's pretend it was empty
             self._load_from_string('')
+        if section_id in self.dirty_sections:
+            # We already created a mutable section for this id
+            return self.dirty_sections[section_id]
         if section_id is None:
             section = self._config_obj
         else:
             section = self._config_obj.setdefault(section_id, {})
         mutable_section = self.mutable_section_class(section_id, section)
         # All mutable sections can become dirty
-        self.dirty_sections.append(mutable_section)
+        self.dirty_sections[section_id] = mutable_section
         return mutable_section
 
     def quote(self, value):

=== modified file 'bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py'
--- bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py	2012-01-27 22:18:07 +0000
+++ bzrlib/plugins/changelog_merge/tests/test_changelog_merge.py	2012-03-28 15:52:30 +0000
@@ -186,7 +186,10 @@
             base_text, True)
         builder.change_contents('clog-id', other=other_text, this=this_text)
         merger = builder.make_merger(merge.Merge3Merger, ['clog-id'])
-        merger.this_branch.get_config_stack().set(
+        # The following can't use config stacks until the plugin itself does
+        # ('this_branch' is already write locked at this point and as such
+        # won't write the new value to disk where get_user_option can get it).
+        merger.this_branch.get_config().set_user_option(
             'changelog_merge_files', 'ChangeLog')
         merge_hook_params = merge.MergeFileHookParams(merger, 'clog-id', None,
             'file', 'file', 'conflict')

=== modified file 'bzrlib/tests/test_branch.py'
--- bzrlib/tests/test_branch.py	2012-03-14 15:26:01 +0000
+++ bzrlib/tests/test_branch.py	2012-03-28 15:52:30 +0000
@@ -661,10 +661,13 @@
         finally:
             copy.unlock()
         self.assertFalse(self.branch.is_locked())
-        result = self.branch.get_config_stack().get('foo')
-        # Bug: https://bugs.launchpad.net/bzr/+bug/948339
-        self.expectFailure('Unlocked branches cache their configs',
-            self.assertEqual, 'bar', result)
+        # Since the branch is locked, the option value won't be saved on disk
+        # so trying to access the config of locked branch via another older
+        # non-locked branch object pointing to the same branch is not supported
+        self.assertEqual(None, self.branch.get_config_stack().get('foo'))
+        # Using a newly created branch object works as expected
+        fresh = _mod_branch.Branch.open(self.branch.base)
+        self.assertEqual('bar', fresh.get_config_stack().get('foo'))
 
     def test_set_from_config_get_from_config_stack(self):
         self.branch.lock_write()
@@ -679,18 +682,21 @@
         self.branch.lock_write()
         self.addCleanup(self.branch.unlock)
         self.branch.get_config_stack().set('foo', 'bar')
-        self.assertEqual('bar',
+        # Since the branch is locked, the option value won't be saved on disk
+        # so mixing get() and get_user_option() is broken by design.
+        self.assertEqual(None,
                          self.branch.get_config().get_user_option('foo'))
 
-    def test_set_delays_write(self):
+    def test_set_delays_write_when_branch_is_locked(self):
         self.branch.lock_write()
         self.addCleanup(self.branch.unlock)
         self.branch.get_config_stack().set('foo', 'bar')
         copy = _mod_branch.Branch.open(self.branch.base)
         result = copy.get_config_stack().get('foo')
-        # Bug: https://bugs.launchpad.net/bzr/+bug/948339
-        self.expectFailure("Config writes are not cached.", self.assertIs,
-                           None, result)
+        # Accessing from a different branch object is like accessing from a
+        # different process: the option has not been saved yet and the new
+        # value cannot be seen.
+        self.assertIs(None, result)
 
 
 class TestPullResult(tests.TestCase):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-03-14 09:30:48 +0000
+++ bzrlib/tests/test_config.py	2012-03-28 15:52:30 +0000
@@ -2860,6 +2860,20 @@
         store.save()
         self.assertEquals(False, self.has_store(store))
 
+    def test_mutable_section_shared(self):
+        store = self.get_store(self)
+        store._load_from_string('foo=bar\n')
+        # FIXME: There should be a better way than relying on the test
+        # parametrization to identify branch.conf -- vila 2011-0526
+        if self.store_id in ('branch', 'remote_branch'):
+            # branch stores requires write locked branches
+            self.addCleanup(store.branch.lock_write().unlock)
+        section1 = store.get_mutable_section(None)
+        section2 = store.get_mutable_section(None)
+        # If we get different sections, different callers won't share the
+        # modification
+        self.assertIs(section1, section2)
+
     def test_save_emptied_succeeds(self):
         store = self.get_store(self)
         store._load_from_string('foo=bar\n')

=== modified file 'bzrlib/tests/test_merge_core.py'
--- bzrlib/tests/test_merge_core.py	2012-02-23 23:26:35 +0000
+++ bzrlib/tests/test_merge_core.py	2012-03-28 15:52:30 +0000
@@ -43,6 +43,7 @@
 
 
 class MergeBuilder(object):
+
     def __init__(self, dir=None):
         self.dir = osutils.mkdtemp(prefix="merge-test", dir=dir)
         self.tree_root = generate_ids.gen_root_id()

=== modified file 'doc/developers/configuration.txt'
--- doc/developers/configuration.txt	2012-01-28 15:37:27 +0000
+++ doc/developers/configuration.txt	2012-03-28 15:52:30 +0000
@@ -98,6 +98,13 @@
 * convert the unicode string provided by the user into a suitable
   representation (integer, list, etc).
 
+If you start migrating a given option to the config stacks, don't stop
+mid-way, all its uses should be covered (tests included). There are some
+edge cases where updates via one API will be not be seen by the other API
+(see http://pad.lv/948339 and http://pad.lv/948344 for details). Roughly,
+the old API always trigger an IO while the new one cache values to avoid
+them. This works fine as long as a given option is handled via a single API.
+
 Adding a new stack
 ------------------
 

=== modified file 'doc/en/release-notes/bzr-2.6.txt'
--- doc/en/release-notes/bzr-2.6.txt	2012-03-16 15:05:05 +0000
+++ doc/en/release-notes/bzr-2.6.txt	2012-03-28 15:52:30 +0000
@@ -117,6 +117,9 @@
 * Fix ``bzr config`` display for ``RegistryOption`` values.
   (Vincent Ladeuil, #930182)
 
+* Option values set on locked branches should be saved only when the branch
+  is finally unlocked. (Vincent Ladeuil, #948339)
+
 Documentation
 *************