Move all mutations into a specific place.

Move all mutations into a specific place.

This makes coherency checks more consistent for DEPS vs .gclient.

It's still not thread safe but the mutation code paths are easier to follow.

R=dpranke@chromium.org
BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=104772

[M-A Ruel, 2011-10-05 00:40:51]

http://codereview.chromium.org/8135019/diff/1/gclient.py
File gclient.py (right):

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode264
gclient.py:264: def setup_requirements(self):
ok you can laugh at me for splitting it again.

[M-A Ruel, 2011-10-06 13:08:40]

ping

[Dirk Pranke, 2011-10-06 19:34:29]

http://codereview.chromium.org/8135019/diff/1/gclient.py
File gclient.py (right):

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode266
gclient.py:266: as a requirement.
Nit: document the return value? It's not obvious that this is returning
something, or what it means if it returns true or false. Also I might rename the
method to something (not sure what) to clarify that.

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode273
gclient.py:273: sibblings = [d for d in self.root.subtree(False) if d.name ==
self.name]
Nit: there's only one "b" in "siblings". Here and the next few lines.

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode394
gclient.py:394: """Parses the DEPS file for this dependency."""
Is this going to be called by multiple threads simultaneously?

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode465
gclient.py:465: def add_dependencies_and_close(self, deps_to_add, hooks):
does it make sense to make this a locked method? Otherwise you're locking and
unlocking in add_dependency() a lot.

[M-A Ruel, 2011-10-06 20:44:19]

http://codereview.chromium.org/8135019/diff/1/gclient.py
File gclient.py (right):

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode266
gclient.py:266: as a requirement.
On 2011/10/06 19:34:29, Dirk Pranke wrote:
> Nit: document the return value? It's not obvious that this is returning
> something, or what it means if it returns true or false. Also I might rename
the
> method to something (not sure what) to clarify that.

documented

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode273
gclient.py:273: sibblings = [d for d in self.root.subtree(False) if d.name ==
self.name]
On 2011/10/06 19:34:29, Dirk Pranke wrote:
> Nit: there's only one "b" in "siblings". Here and the next few lines.

fixed

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode394
gclient.py:394: """Parses the DEPS file for this dependency."""
On 2011/10/06 19:34:29, Dirk Pranke wrote:
> Is this going to be called by multiple threads simultaneously?

Not starting with http://codereview.chromium.org/8143022.

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode465
gclient.py:465: def add_dependencies_and_close(self, deps_to_add, hooks):
On 2011/10/06 19:34:29, Dirk Pranke wrote:
> does it make sense to make this a locked method? Otherwise you're locking and
> unlocking in add_dependency() a lot. 

Yes and that's the purpose. The lock must be kept as little as possible, and the
code guarantees internal consistency all the time the lock is released.

[Dirk Pranke, 2011-10-06 20:50:45]

lgtm w/ further comments. Up to you if you want to change them or not.

http://codereview.chromium.org/8135019/diff/1/gclient.py
File gclient.py (right):

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode465
gclient.py:465: def add_dependencies_and_close(self, deps_to_add, hooks):
On 2011/10/06 20:44:19, Marc-Antoine Ruel wrote:
> On 2011/10/06 19:34:29, Dirk Pranke wrote:
> > does it make sense to make this a locked method? Otherwise you're locking
and
> > unlocking in add_dependency() a lot. 
> 
> Yes and that's the purpose. The lock must be kept as little as possible, and
the
> code guarantees internal consistency all the time the lock is released.

Well, there's nothing in setup_requirements that can block, right()? So I'm
pretty sure you'll hold the GIL the whole time and only one thread will be
actually executing add_dependencies_and_close() at a time, so it just seems like
it might be a bit more efficient. Not a big deal, though.

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode470
gclient.py:470: self._mark_as_parsed(hooks)
I thought I had mentioned this in the earlier email, but I don't see the comment
... I'd probably move _mark_as_parsed out of this routine and after line 462
(and then rename this routine).

[M-A Ruel, 2011-10-06 20:57:35]

http://codereview.chromium.org/8135019/diff/1/gclient.py
File gclient.py (right):

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode465
gclient.py:465: def add_dependencies_and_close(self, deps_to_add, hooks):
On 2011/10/06 20:50:46, Dirk Pranke wrote:
> Well, there's nothing in setup_requirements that can block, right()?

Yes there is something. setup_requirements() does a tree traversal, which calls
.dependencies on all objects. So no lock must be held when this function is
called.

Note that I've finally coded a fix for parallel processing, which I'll send you
shortly once I clean up the code. This will mark the end of this journey in
refactoring the crap out of this class.

http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode470
gclient.py:470: self._mark_as_parsed(hooks)
On 2011/10/06 20:50:46, Dirk Pranke wrote:
> I thought I had mentioned this in the earlier email, but I don't see the
comment
> ... I'd probably move _mark_as_parsed out of this routine and after line 462
> (and then rename this routine).

If I'd do that, I'd have to add this call at the other call site at line 802.
That's why I preferred to keep the call inside add_dependencies_and_close(). Not
much opinion though.

[Dirk Pranke, 2011-10-06 20:59:34]

Ah, okay (to both points).

On Thu, Oct 6, 2011 at 1:57 PM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/8135019/diff/1/gclient.py
> File gclient.py (right):
>
> http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode465
> gclient.py:465: def add_dependencies_and_close(self, deps_to_add,
> hooks):
> On 2011/10/06 20:50:46, Dirk Pranke wrote:
>>
>> Well, there's nothing in setup_requirements that can block, right()?
>
> Yes there is something. setup_requirements() does a tree traversal,
> which calls .dependencies on all objects. So no lock must be held when
> this function is called.
>
> Note that I've finally coded a fix for parallel processing, which I'll
> send you shortly once I clean up the code. This will mark the end of
> this journey in refactoring the crap out of this class.
>
> http://codereview.chromium.org/8135019/diff/1/gclient.py#newcode470
> gclient.py:470: self._mark_as_parsed(hooks)
> On 2011/10/06 20:50:46, Dirk Pranke wrote:
>>
>> I thought I had mentioned this in the earlier email, but I don't see
>
> the comment
>>
>> ... I'd probably move _mark_as_parsed out of this routine and after
>
> line 462
>>
>> (and then rename this routine).
>
> If I'd do that, I'd have to add this call at the other call site at line
> 802. That's why I preferred to keep the call inside
> add_dependencies_and_close(). Not much opinion though.
>
> http://codereview.chromium.org/8135019/
>

[commit-bot: I haz the power, 2011-10-10 20:00:15]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/maruel@chromium.org/8135019/5001

[commit-bot: I haz the power, 2011-10-10 20:06:10]

Change committed as 104772