Page MenuHomePhabricator
Differential D9361

Batch up SQL operations in the `./bin/repository parents` script.
ClosedPublic
Actions

Authored by joshuaspence on Jun 3 2014, 5:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 12:03 AM
Unknown Object (File)
Thu, Dec 12, 6:41 AM
Unknown Object (File)
Mon, Dec 9, 12:49 AM
Unknown Object (File)
Sat, Nov 30, 12:43 PM
Unknown Object (File)
Tue, Nov 26, 8:01 AM
Unknown Object (File)
Tue, Nov 26, 8:01 AM
Unknown Object (File)
Tue, Nov 26, 8:01 AM
Unknown Object (File)
Tue, Nov 26, 8:01 AM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T5255: Batch up SQL operations in the `./bin/repository parents` script
Commits
Restricted Diffusion Commit
rP1503840cd945: Batch up SQL operations in the `./bin/repository parents` script.
Summary

Fixes T5255. Currently the ./bin/repository parents workflow is quite slow. Batching up the SQL operations should make the workflow seem much faster.

Test Plan

Not yet tested.

Diff Detail

Repository
rP Phabricator
Branch
repo_parents
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 828
Build 828: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 22289.Jun 3 2014, 5:23 PM
joshuaspence retitled this revision from to Batch up SQL operations in the `./bin/repository parents` script..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence added a task: T5255: Batch up SQL operations in the `./bin/repository parents` script.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJun 3 2014, 5:23 PM
Herald added subscribers: Korvin, epriestley. · View Herald Transcript
Harbormaster completed remote builds in B825: Diff 22289.Jun 3 2014, 5:24 PM
epriestley edited edge metadata.Jun 3 2014, 5:24 PM

We probably need PhabricatorLiskDAO::chunkSQL() around the actual queries to prevent overflowing the packet limit. This looks reasonable otherwise.

joshuaspence updated this revision to Diff 22290.Jun 3 2014, 5:34 PM
joshuaspence edited edge metadata.
  • Use PhabricatorLiskDAO::chunkSQL
Harbormaster completed remote builds in B826: Diff 22290.Jun 3 2014, 5:35 PM
joshuaspence added a comment.Jun 3 2014, 5:36 PM

I'm not sure whether this is actually helping my initial issue... the whole process still seems very slow. I am investigating now.

joshuaspence added a comment.Jun 3 2014, 5:36 PM

(which means the SELECTs probably need to be batched as well)

joshuaspence added a comment.Jun 3 2014, 5:49 PM

In fact, I'm not convinced that this is much faster at all...

Before

> time ./bin/repository parents PP
Rebuilding "rPP"...
Rebuilding branch "production"...
Found 3,714 total commit(s); updating...
Done.                                                                         
                                                                              
real	3m16.982s
user	0m12.769s
sys	2m14.456s

After

time ./bin/repository parents PP
Rebuilding "rPP"...
Rebuilding branch "production"...
Found 3,714 total commit(s); updating...
Done.                                                                         
                                                                              
real	3m1.038s
user	0m11.441s
sys	2m17.209s
epriestley added a comment.Jun 3 2014, 5:58 PM

With select batching, this drops from 30s to ~2s for me locally when run on the Phabricator repo:

https://secure.phabricator.com/differential/diff/22292/

joshuaspence updated this revision to Diff 22293.Jun 3 2014, 6:03 PM

Batch SELECT statements as well.

Harbormaster completed remote builds in B828: Diff 22293.Jun 3 2014, 6:05 PM
joshuaspence added a comment.Jun 3 2014, 6:15 PM

This didn't seem to help either.

> time ./bin/repository parents PP        
Rebuilding "rPP"...
Rebuilding branch "production"...
Found 3,714 total commit(s); updating...
Done.                                                                         
                                                                              
real	3m11.955s
user	0m11.469s
sys	2m31.537s
epriestley added a comment.Jun 3 2014, 6:15 PM

(Is the fully batched version faster for you?)

epriestley added a comment.Jun 3 2014, 6:15 PM

Hmm, try with --trace?

epriestley added a comment.Jun 3 2014, 6:16 PM

omg i figured it out hahaha

joshuaspence added a comment.Jun 3 2014, 6:19 PM
In D9361#20, @epriestley wrote:

Hmm, try with --trace?

Interestingly, --trace caused the script to terminate after a few seconds...

epriestley added a comment.Jun 3 2014, 6:20 PM

D9363

joshuaspence added a comment.Jun 3 2014, 6:22 PM
In D9361#23, @epriestley wrote:

D9363

With D9363 applied:

time ./bin/repository parents PP
Rebuilding "rPP"...
Rebuilding branch "production"...
Found 3,714 total commit(s); updating...
Done.                                                                         
                                                                              
real	0m1.545s
user	0m0.712s
sys	0m0.624s

:D

joshuaspence added a comment.Jun 3 2014, 6:26 PM

This diff may not be required after D9363, with just D9363 applied there is a 6 times speed improvement for me:

> time ./bin/repository parents PP        
Rebuilding "rPP"...
Rebuilding branch "production"...
Found 3,714 total commit(s); updating...
Done.                                                                         
                                                                              
real	0m32.059s
user	0m3.736s
sys	0m9.913s
epriestley accepted this revision.Jun 3 2014, 6:27 PM
epriestley edited edge metadata.

I'm going to grab this too since it does provide a real performance increase and the complexity is reasonably well isolated, it just pales in comparison to the progress bar perf issue.

This revision is now accepted and ready to land.Jun 3 2014, 6:27 PM
epriestley closed this revision.Jun 3 2014, 6:28 PM
epriestley updated this revision to Diff 22298.

Closed by commit rP1503840cd945 (authored by @joshuaspence, committed by @epriestley).

Revision Contents
Changeset List

PathSize
src/
applications/
repository/
management/
63 lines

Diff 22293

View Options

src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php

Loading...
Phabricator · Built by Phacility