Disabled threaded index-pack for known difficult repositories.

Disabled threaded index-pack for known difficult repositories.

BUG=349576
R=mmoss@google.com

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259164

[iannucci, 2014-03-24 23:45:09]

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py#newcode1007
gclient_utils.py:1007: pack_threads = 1 if url in THREADED_INDEX_PACK_BLACKLIST
else 4
Not sure about the hard-coded 4 default. What about:

PACK_THREAD_LIMIT = {
  url: num
}

if url in PACK_THREAD_LIMIT:
  args += ['-c', 'pack.threads=%d' % PACK_THREAD_LIMIT[url]]


?

Then you could put it as 4 for src and blink, 1 for this bad repo, and then let
git figure it out for everything else?

[szager1, 2014-03-24 23:56:44]

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py#newcode1007
gclient_utils.py:1007: pack_threads = 1 if url in THREADED_INDEX_PACK_BLACKLIST
else 4
On 2014/03/24 23:45:10, iannucci wrote:
> Not sure about the hard-coded 4 default. What about:
> 
> PACK_THREAD_LIMIT = {
>   url: num
> }
> 
> if url in PACK_THREAD_LIMIT:
>   args += ['-c', 'pack.threads=%d' % PACK_THREAD_LIMIT[url]]
> 
> 
> ?
> 
> Then you could put it as 4 for src and blink, 1 for this bad repo, and then
let
> git figure it out for everything else?

I don't have any reason to believe that 4 is more appropriate for src and blink
than for other repos.  In my experiments, 4 was the point of diminishing
returns, but I didn't test it for all deps.

git will just look for a pack.threads config; if it fails to find one, then it
defaults to 3.  So, the only meaningful effect of leaving out the pack.threads
argument on the command-line is to allow users to override it in their
~/.gitconfig.  Which is totally legit for users who are inclined to tinker with
such things.

[mmoss, 2014-03-25 00:21:43]

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py#newcode33
gclient_utils.py:33: # These repos are known to cause OOM errors on 32-bit
platforms, due the the
It would be nice if this only kicked in under those conditions, but I guess not
a big deal for just this repo.

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py#newcode1007
gclient_utils.py:1007: pack_threads = 1 if url in THREADED_INDEX_PACK_BLACKLIST
else 4
> defaults to 3.  So, the only meaningful effect of leaving out the pack.threads
> argument on the command-line is to allow users to override it in their
> ~/.gitconfig.  Which is totally legit for users who are inclined to tinker
with
> such things.

Yeah, only adding the command-line arg when "necessary" seems like the best bet,
and leave it to default/user value the rest of the time (possibly with "highly
recommended" values coming from etc/gitconfig).

[szager1, 2014-03-25 00:40:24]

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py
File gclient_utils.py (right):

https://codereview.chromium.org/210063005/diff/1/gclient_utils.py#newcode1007
gclient_utils.py:1007: pack_threads = 1 if url in THREADED_INDEX_PACK_BLACKLIST
else 4
On 2014/03/25 00:21:44, mmoss wrote:
> > defaults to 3.  So, the only meaningful effect of leaving out the
pack.threads
> > argument on the command-line is to allow users to override it in their
> > ~/.gitconfig.  Which is totally legit for users who are inclined to tinker
> with
> > such things.
> 
> Yeah, only adding the command-line arg when "necessary" seems like the best
bet,
> and leave it to default/user value the rest of the time (possibly with "highly
> recommended" values coming from etc/gitconfig).

OK, the code now only sets pack.threads on the command line for the  blacklisted
repos.

[mmoss, 2014-03-25 01:04:36]

lgtm

[szager1, 2014-03-25 06:02:10]

Committed patchset #2 manually as r259164 (presubmit successful).