Summary:
This is kind of expensive and can be significant on, e.g., the
Maniphest task list view. Do a little more caching and some clever nonsense to
improve performance.
Test Plan:
Local cost on Maniphest "all tasks" view for this method dropped from
##82,856us## to ##24,607us## on 9,061 calls.
I wrote some unit test / microbenchmark things:
public function testGetIDCost() {
$u = new PhabricatorUser();
$n = 100000;
while ($n--) {
$u->getID();
}
$this->assertEqual(1, 1);
}
public function testGetCost() {
$u = new PhabricatorUser();
$n = 100000;
while ($n--) {
$u->getUsername();
}
$this->assertEqual(1, 1);
}
public function testSetCost() {
$u = new PhabricatorUser();
$n = 100000;
while ($n--) {
$u->setID(1);
}
$this->assertEqual(1, 1);
}
Before:
PASS 598ms testSetCost
PASS 584ms testGetCost
PASS 272ms testGetIDCost
After:
PASS 170ms testSetCost
PASS 207ms testGetCost
PASS 29ms testGetIDCost
Also, ran unit tests.
Reviewers: nh, btrahan, jungejason
Reviewed By: nh
CC: aran, epriestley, nh
Differential Revision: https://secure.phabricator.com/D1291
Summary: While we eventually need a plan to make this less of a toy black hole,
we can support DELETE now -- it's just SELECT that's tricky.
Test Plan: Ran unit tests.
Reviewers: zeeg, jungejason, btrahan
Reviewed By: btrahan
CC: aran, btrahan
Differential Revision: 1226
Summary:
- Use DifferentialRevisionQuery, not DifferentialRevisionListData, to select
revisions.
- Make UI simpler (I hope?) and more flexible, similar to Maniphest. It now
shows "Active", "Revisions", "Reviews" and "Subscribed" instead of a hodge-podge
of miscellaneous stuff. All now really has all revisions, not just open
revisions.
- Allow views to be filtered and sorted more flexibly.
- Allow anonymous users to use the per-user views, just don't default them
there.
NOTE: This might have performance implications! I need some help evaluating
them.
@nh / @jungejason / @aran, can one of you run some queries agianst FB's corpus?
The "active revisions" view is built much differently now. Before, we issued two
queries:
- SELECT (open revisions you authored that need revision) UNION ALL (open
revisions you are reviewing that need review)
- SELECT (open revisions you authored that need review) UNION ALL (open
revisions you are reviewing that need revision)
These two queries generate the "Action Required" and "Waiting on Others" views,
and are available in P247.
Now, we issue only one query:
- SELECT (open revisions you authored or are reviewing)
Then we divide them into the two tables in PHP. That query is available in P246.
On the secure.phabricator.com data, this new approach seems to be much better
(like, 10x better). But the secure.phabricator.com data isn't very large. Can
someone run it against Facebook's data (using a few heavy-hitting PHIDs, like
ola or something) to make sure it won't cause a regression?
In particular:
- Run the queries and make sure the new version doesn't take too long.
- Run the queries with EXPLAIN and give me the output maybe?
Test Plan:
- Looked at different filters.
- Changed "View User" PHID.
- Changed open/all.
- Changed sort order.
- Ran EXPLAIN / select against secure.phabricator.com corpus.
Reviewers: btrahan, nh, jungejason
Reviewed By: btrahan
CC: cpiro, aran, btrahan, epriestley, jungejason, nh
Maniphest Tasks: T586
Differential Revision: 1186
test
Summary: @jungejason reported seeing test failures here. I can't reproduce them
and my read of the code doesn't suggest why they might be happening, but add a
little more debug info in hopes of chasing this down.
Test Plan:
- Ran test in a loop for a long time, couldn't get it to fail.
- Changed assertEquals() condition to force test to fail, verified output
message was informative.
Reviewers: jungejason, btrahan
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 1212
Summary:
I broke this a little bit in my overzealous D1174, since this block validates
both '%nd' (nullable integer) and '%d' (non-nullable integer).
Clean up the conditional checks so we catch the bad case ('%d' on a PHID
converting to 0) but let the good case ('%nd' with null) through.
Test Plan: Unit tests failed; applied patch; unit tests pass.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, btrahan
Maniphest Tasks: T670
Differential Revision: 1201
Summary: See T547. One of these I just missed in D1000; the comment change just
makes it easier to audit use of hash functions by cleaning up "grep" output.
Test Plan: Ran isolation unit test.
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 1124
Summary:
I added some type checks in D990 to make sure $columns is an array, but was
overzealous and forgot that loadRawDataWhere needs to be able to take null
as $columns.
Test Plan:
Loaded phabricator and saw the error "Argument 1 passed to LiskDAO::loadRawDataWhere() must be an instance of array, null given" go away
Reviewers: epriestley
CC:
Differential Revision: 991
Summary:
Change the differential typeahead to only load columns that it needs. To do
this, I also enabled partial objects for PhabricatorUser (and made necessary
changes to support this). I also changed the functionality of Lisk's loadColumns
to either accept columns as multiple string arguments or a single array of
strings.
Test Plan:
With tokenizer.ondemand set to false, checked that the typeahead loaded and I
can type multiple people's names. Set tokenizer.ondemand to true and tried
again. In both cases, the typeahead worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran, epriestley, nh
Differential Revision: 990
Summary:
Added loadColumns, loadColumnsWhere instance methods to Lisk, so when you only
need some fields of your object loaded, you can do so. This will be useful for
places where we fetch a large number of rows, but only care about a few columns.
In that situation, these functions can be used so the db doesn't have to return
as much data.
Test Plan:
Loaded a typeahead to check that the existing lisk functions still work.
Modified typeahead to fetch data using loadColumns instead of loadAll and
checked that it still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, nh, jungejason
Differential Revision: 947
Summary:
This diff changes the way Lisk should be used for custom setters and getters,
changing it from having subclasses of Lisk implement their custom setter or
getter to having them override the readField and writeField methods (which get
called by the getters and setters). This diff also has a configurable option
to throw an exception if a subclass of Lisk implements a custom setter or
getter.
Test Plan:
Without the config set to throw, tested in sandbox by browsing differential
and playing with the differential typeahead. With the config set to throw,
tried to load a phabricator page and saw in the error log an exception thrown
by Lisk because of custom getters in PhabricatorUser.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 974
Summary: These queries are safe to run without a CSRF token, and we need them
for the query analyzer in DarkConsole.
Test Plan: "Analyze Query Plans" works again.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, epriestley, nh
Differential Revision: 895
Summary:
Provide a catchall mechanism to find unprotected writes.
- Depends on D758.
- Similar to WriteOnHTTPGet stuff from Facebook's stack.
- Since we have a small number of storage mechanisms and highly structured
read/write pathways, we can explicitly answer the question "is this page
performing a write?".
- Never allow writes without CSRF checks.
- This will probably break some things. That's fine: they're CSRF
vulnerabilities or weird edge cases that we can fix. But don't push to Facebook
for a few days unless you're prepared to deal with this.
- **>>> MEGADERP: All Conduit write APIs are currently vulnerable to CSRF!
<<<**
Test Plan:
- Ran some scripts that perform writes (scripts/search indexers), no issues.
- Performed normal CSRF submits.
- Added writes to an un-CSRF'd page, got an exception.
- Executed conduit methods.
- Did login/logout (this works because the logged-out user validates the
logged-out csrf "token").
- Did OAuth login.
- Did OAuth registration.
Reviewers: pedram, andrewjcg, erling, jungejason, tuomaspelkonen, aran,
codeblock
Commenters: pedram
CC: aran, epriestley, pedram
Differential Revision: 777
query
Summary:
- Provide an example unit test, and document it.
- Document database isolation better.
- When we issue an unsimulated query to the isolated connection, throw a
helpful message.
- Pygments is complaining about my madeup "lang=demo", change it to
"lang=text".
Test Plan:
- Ran the unit test (sanity check).
- Ran all other unit tests (verify I didn't break isolation).
- Added a queryfx(..., 'SELECT 1') to a test and verified it throws.
- Read the documentation.
Reviewed By: edward
Reviewers: edward, jungejason, tuomaspelkonen, aran
CC: aran, edward
Differential Revision: 773
Summary:
- Services: Show summary panel of total service call costs and relative page weight.
- Services: Add "Analyze Query Plans" button, which issues EXPLAIN for each query and flags problems.
- XHPRof: iframe the profile.
Test Plan: Used the new query plan analysis to find missing keys causing table scans, see D627.
Reviewers: jungejason, tuomaspelkonen, aran
CC:
Differential Revision: 628
Summary:
Get rid of the Phabricator-level DarkConsole-specific API and use the more
general Phutil-level one.
Test Plan:
Loaded DarkConsole services plugin, viewed Diffusion, got execs in the trace.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 293
Summary:
Currently you can still punch through Lisk isolation by calling
establishConnection(), and we do that all over the place. Rename getConnection()
to establishConnection() so that all existing callers are safe, and rename
establishConnection() to establishLiveConnection() so that it's not surprising
when this fails to stub in unit tests.
Not wedded to the name if anyone thinks "establishExternalConnection" or
something is clearer.
Test Plan:
Loaded site, browsed around, ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 201
Summary:
Allow Lisk to be put into process-isolated mode which establishes
only isolated connections. By default, put it into this mode when running
unit tests. Build some simple unit tests around object insertion and
updating.
NOTE: The one flaw in this is that $dao->establishConnection() still
punches through the isolation layer. I need to do an API change to fix this
though so I'm holding it for now. It will probably just rename getConnection()
to establishConnection() and then rename establishConnection() to something
scary like establishLiveExternalConnection().
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 194
Summary:
This provides a new connection which doesn't connect to
anything, so effects can be isolated to the current process (for
unit testing).
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 193
Summary:
In a basically reasonable configuration where you connect
with a non-privileged user from the web workflow, upgrade_schema.php
won't have enough privileges. Allow the user to override the normal
auth with -u and -p.
Test Plan:
Tried to do a schema upgrade with an underprivileged user,
got a useful error message instead of garbage.
Reviewed By: Girish
Reviewers: Girish, davidrecordon, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, Girish
Differential Revision: 191
Summary:
This permits individual deployments to better configure their
database configuration, e.g. to allow more dynamic configuration that reacts
to database moves or master/slave replication.
Test Plan:
Browse
Reviewed By: epriestley
Reviewers: Girish, epriestley
CC: aran, epriestley
Differential Revision: 183
Summary:
Add ability to define mysql slaves and then use that connection on 'r'
connection modes. 'w' connections go to the master server.
Test Plan:
- php -l and checkModule
- worked in my devbox
Reviewed By: jungejason
Reviewers: dpepper, tuomaspelkonen, jungejason
CC: jungejason, aran
Revert Plan:
sure
Differential Revision: 175
Summary: Basic scaffolding for repository tracking, plus daemon infrastructure
(Timelines, Cursors) and some fixes (memory usage, mysql_connect() junk).
Test Plan: parsed Javelin git commit history via daemon
Reviewers:
CC:
