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
Lint
Lint Skipped
Unit
Tests Skipped

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 22298

View Options

src/applications/repository/management/PhabricatorRepositoryManagementParentsWorkflow.php

Loading...
Phabricator · Built by Phacility