Summary:
D477 added functionality to the project list view but had a couple of
performance issues that I missed in review, because it took the query count for
the page from around 3 to as many as 300, including up to 100 heavyweight search
index queries.
This fixes the two simple N+1 query problems. This general pattern of data
access often occurs:
COUNTEREXAMPLE
$cats = load_cats();
foreach ($cats as $cat) {
$cats_hats = load_hats_for_cat($cat);
// ...
}
But this issues "N+1" queries, i.e. if you load 100 cats you issue 101 queries.
It is faster to group the queries instead:
$cats = load_cats();
$hats = load_all_hats_for_these_cats($cats);
foreach ($cats as $cat) {
$cats_hats = $hats[$cat->getID()];
}
MySQL can execute one query which returns all the results much faster than 100
queries which return one result, especially if the database is not local (i.e.,
over the network).
However, this doesn't save a ton of time. The bigger issue is that I didn't have
the right keys on the relationship tables in the search engine. This adds them,
and reduces the search engine lookup cost from 25-80ms (for
secure.phabricator.com) down to 1-3ms.
I still probably want to get this out of the loop at some point but it's okay
for now and the page loads in a few ms rather than taking more than a second.
Test Plan:
Used "services" tab, "xhprof" and "EXPLAIN" to analyze page performance. I
measured these changes:
- Query count: 1 + (3 * N projects) -> 3 + (N projects) (e.g., 301 -> 103)
- Total time spent querying, ignoring search indexes: 40ms (local.aprhont.com)
-> 20ms (local.aphront.com)
- Cost for search index query: 25-80ms (secure.phabricator.com) -> 1-3ms
Reviewed By: cadamo
Reviewers: cadamo, aran, jungejason, tuomaspelkonen
CC: aran, cadamo, epriestley
Differential Revision: 485
Summary:
If you "chmod +x" a file and then generate a diff and click "show 20 lines" on
that diff, you get another janky copy of the property table. Render the property
table only for top-level rendering requests.
This started happening after D409, which fixed the far-more-obvious bug of these
things never showing up. We must have changed the logic at some point since this
side effect was surprising to me. :P
Test Plan:
Created a diff with changes and +x, clicked "show 20 lines".
- Original diff had file property header table thing, showing the +x.
- New context brought in by "show 20 lines" didn't have it anymore.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 482
Summary:
This ended up being pretty hard to see, make it a bit easier.
Test Plan:
Focused things using the keyboard reticle.
Reviewed By: tomo
Reviewers: tomo, moskov, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tomo
Differential Revision: 483
Summary:
For some reason, Webkit parses the completely made-up "background-position-y"
property. Firefox does not. Use a real property instead of a creative one that
doesn't exist.
Test Plan:
Hovered over "Phabricator" logo in Firefox, Safari.
Reviewed By: codeblock
Reviewers: codeblock, aran, jungejason, tuomaspelkonen
CC: aran, codeblock
Differential Revision: 484
Summary:
Without this, user creation throws an exception when trying to insert NULL into
a non-NULL field.
Test Plan:
Created a new user.
Reviewed By: fratrik
Reviewers: fratrik, toulouse, aran, jungejason, tuomaspelkonen
CC: aran, fratrik
Differential Revision: 480
Summary: Added some change on the project's list view, to show information about
active tasks, population, etc. Also modified the "profile view", and added a class "PhabricatorProfileView" to render the profile, both on projects and users.
Test Plan: play around the project directory :)
Reviewers: epriestley ericfrenkiel
CC:
Differential Revision: 477
Summary:
create the event
Test Plan:
checked the timeline event was consumed successfully by
facebook daemon when the revision is created from the webUI or arc
command line.
Reviewed By: epriestley
Reviewers: epriestley, aran
CC: aran, epriestley
Differential Revision: 478
Summary:
Provides a new workflow for making it non-horrible to install certificates.
Basically you run "arc install-certificate" and then copy/paste a short token
off a webpage and it does the ~/.arcrc edits for you.
Test Plan:
Installed certificates, used bad tokens, hit rate limiting.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 460
Summary:
The owners of a revision are only really the reviewers when the revision is in
NEEDS_REVIEW.
Also build a raw indexed document viewer so you can look at the index of a
document from the web interface.
Finally, reindex revisions when comments are added, not just when the revision
itself is edited.
Test Plan:
Toggled abandon/reclaim on a revision and verified the relationships indexed
properly.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley, jungejason
Differential Revision: 470
Summary:
The MySQL MyISAM fulltext backend defaults to a minimum length of 4, but many
3-letter words are useful search terms. Provide instructions for configuring
MySQL to reduce the auto-stopword length.
Test Plan:
Followed instructions, searched for a 3-letter term and got a hit.
Reviewed By: jungejason
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, jungejason
Differential Revision: 471
Summary:
'Create Diff' with whitespace mode 'ignore-all' is borken, because the
line numbers get mixed up when creating a second diff for the
whitespace changes.
This should be fixed correctly at some point, but currently the
whitespace 'ignore-all' says 'Huge mess' in the comments and I didn't
want to make the mess any bigger.
Test Plan:
Tested that 'Create Diff' showed the diff correctly and a previously
created diff looked correct once the cache was disabled.
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: aran, epriestley
Differential Revision: 473
Summary:
Allows user-configurable timezones. Adds a preference panel, and migrates to the
new date rendering in easily-modified areas of the code. ***In progress***.
Test Plan:
Check database to make sure the field is being changed when the settings are
changed; check affected views to see how they render times.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley, toulouse
Differential Revision: 475
Summary:
Paste was cutting off lines when they extended past the width of the screen.
Also change a pair of double quotes to single quotes, to follow convention.
Test Plan:
Tried this in Firefox 4 and Chrome.
Reviewers:
epriestley, toulouse
CC:
Differential Revision: 469
Summary:
Make it slightly more clear how to go about this.
Test Plan:
Generated and reviewed the documentation.
Reviewed By: moskov
Reviewers: moskov, aran, jungejason, tuomaspelkonen
CC: aran, moskov
Differential Revision: 467
Differential
Summary:
Make some display stuff more consistent.
Test Plan:
Looked at a task and a revision.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 462
Summary:
Permit "j" and "k" to cycle through individual changeblocks, similar to how this
feature works in ReviewBoard. This still needs a bunch of refinement but it's
getting closer to being useful.
Also moved reticle underneath the table so you can click links through it (derp
derp).
Test Plan:
Used "j" and "k" to cycle through individual changes.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: moskov, aran, epriestley
Differential Revision: 426
Summary:
Allow duplicate tasks to be selected and merged in Maniphest.
I didn't create a separate transaction type for this because that implies a
bunch of really complicated rules which I don't want to sort out right now
(e.g., do we need to do cycle detection for merges? If so, what do we do when we
detect a cycle?) since I think it's unnecessary to get right for the initial
implementation (my Tasks merge implementation was similar to this and worked
quite well) and if/when we eventually need the metadata to be available in a
computer-readable form that need should inform the implementation.
Plenty of room for improvement here, of course.
Test Plan:
Merged duplicate tasks, tried to perform invalid merge operations (e.g., merge a
task into itself).
Tested existing attach workflows (task -> revision, revision -> task).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran
Differential Revision: 459
Summary:
See attached tasks. See D459 for the ability to merge tasks.
Test Plan:
Looked at posted and unposted inline comments.
Reviewed By: aran
Reviewers: edward, viyer, aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 461
Summary:
For some unknown reasons, our test framework fails to update test
results occasionally. Instead of throwing a nasty exception, log
the error message. This will help us to figure out the real reason
behind the error.
Test Plan:
arc lint.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: epriestley, aran
Differential Revision: 463
Summary:
This implements a paste.info conduit method, for retrieving info about a paste. Imagine that.
Test Plan:
[ricky@rhelpad01 method]$ echo '{"paste_id":1}' | arc call-conduit --conduit-uri=http://phabricator.local/api/ paste.info
{"error":null,"errorMessage":null,"response":{"id":"1","phid":"PHID-PSTE-10934f3df8ed33c06555","authorPHID":"PHID-USER-9d03e8fa47516d37dc92","filePHID":"PHID-FILE-e85f6a72c773d24f4981","title":"test.php","dateCreated":"1307731614"}}
Reviewers:
epriestley
CC:
Differential Revision: 458
Summary:
Tweaks to the paste app:
- I realized that unlike all the other apps, it makes more sense for the
default view of this one to be "create paste" instead of "list pastes" since
when you access the application directly you are most often wanting to share
something. Swap list out of the default slot and make edit the default.
- Make the textarea bigger (usability).
- Allow you to copy an existing paste.
- Implement 'raw view'.
- Tweak/adjust list view (usability, formatting).
- Tweak page titles.
Test Plan:
Created, copied, and listed pastes. Viewed raw paste. Created an invalid paste.
Tried to create a copy of a nonexistant paste.
Reviewed By: codeblock
Reviewers: codeblock, jungejason, aran, tuomaspelkonen
CC: aran, epriestley, codeblock
Differential Revision: 456
Summary:
Show how many tasks are in each group. This number is useful when you are
insufficiently depressed.
Test Plan:
Looked at maniphest task list.
Reviewed By: toulouse
Reviewers: aran, jungejason, tuomaspelkonen, toulouse
CC: anjali, aran, toulouse
Differential Revision: 457
Summary:
Add a note on configuring Lamson to the configuring inbound email article.
Test Plan:
Generated and proofread the documentation.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, epriestley
Differential Revision: 442
Summary:
A few tweaks to hsb's Countdown implementation:
- Allow the page to be rendered "chromeless", suitable for display on one of
the dozens of monitors everyone has laying around.
- Show title of countdown in deletion dialog.
- When creating a new countdown default to time(), not Dec 31, 1969.
- Add extra "/" after editing to avoid needless redirect.
- Tweak some page titles.
- Show countdown author in list view.
- Highlight tab in list view.
- Tweak menu copy.
- Link countdown title in list view, separate buttons into different columns
so they pick up padding.
Test Plan:
Created, edited and deleted a timer. Viewed a timer and toggled chrome mode.
Viewed timer list.
Reviewed By: hsb
Reviewers: hsb, aran, jungejason, tuomaspelkonen
CC: aran, hsb, epriestley
Differential Revision: 454
Summary:
See <http://www.facebook.com/epriestley/posts/923755264939>. The favicon is too
complicated and not distinct. Use a quarter-slice of the gear instead and also I
resized it better in Photoshop.
Test Plan:
I'm a faaaaaabulous designer!
Reviewed By: aran
Reviewers: rayhe, tuomaspelkonen, jungejason, aran, tomo
CC: aran, epriestley
Differential Revision: 455
Summary:
Nicks can contain more characters than were allowed. The new regex is ugly,
but should include most of them.
Test Plan:
Joined the bot as phabot-c0d`eb1^ock successfully.
Reviewers:
epriestley
CC:
Differential Revision: 453
Summary:
remove accessing the db config info directly. Use
DatabaseConfigurationProvider instead. Also fixed a minor issue where
different number of newlines are output in PhabricatorSetup.php's output.
Test Plan:
executed upgrade_schema.php; executed PhabricatorSetup.php by
setting 'phabricator.setup' to true.
Reviewed By: epriestley
Reviewers: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 443
Summary:
Point users at the install scripts. Fix some common points of confusion and
update some other documentation points.
The links point to a directory which ultimately serves these scripts out of the
git checkout on the machine:
http://phabricator.com/rsrc/install/install_rhel-derivs.shhttp://phabricator.com/rsrc/install/install_ubuntu.sh
Test Plan:
Read documentation.
Reviewed By: aran
Reviewers: codeblock, jungejason, tuomaspelkonen, aran
Commenters: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 439
Summary:
You don't need these anymore after D444.
Test Plan:
Generated documentation.
Reviewed By: aran
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 445
Summary:
Make 'gd' an explicit optional dependency, test for it in setup, and make the
software behave correctly if it is not available.
When generating file thumnails, provide reasonable defaults and behavior for
non-image files.
Test Plan:
Uploaded text files, pdf files, etc., and got real thumbnails instead of a
broken image.
Simulated setup and gd failures and walked through setup process and image
fallback for thumbnails.
Reviewed By: aran
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 446
Summary:
Make it easier to find obvious problems in daemons by letting them run
undaemonized in the console without requiring the user to know the magical
incantations for loading libraries, etc.
Test Plan:
Ran "phd debug nice", simulated some failures (e.g., bringing down Phabricator,
daemon fatal) and got useful error messages.
Reviewed By: jungejason
Reviewers: toulouse, jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 448
Summary:
Some day, maybe close the existing dialog too but there's no public method on
JX.Workflow for that right now.
Test Plan:
Hit "??????", then "esc", got back to the page instead of just popping a deep
stack.
Reviewed By: tomo
Reviewers: tomo
CC: aran, tomo
Differential Revision: 450
errors.
Summary:
Make sure reviewers know what they are doing.
Test Plan:
Tested with different diffs that had lint and unit problems.
Reviewed By: epriestley
Reviewers: epriestley, jungejason
CC: grglr, aran, epriestley, tuomaspelkonen
Differential Revision: 432
Summary:
Provide a quick workflow for adding a new project. This ended up being sort of
complicated because we don't currently put forms in dialogs. I separated the
actual <form /> tag out of the display/layout of AphrontFormView to enable this
(the dialog is itself a form).
Limitations: if you create a new project and then remove it, it won't appear in
the tokenizer until you reload the page. We need to add the ability for the
datasource to drop its cache to enable this, which is super complicated.
Test Plan:
Used "Create new project" to add a new project when creating a task.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: anjali, aran, epriestley
Differential Revision: 422
Summary:
- Make the instructional text generally more useful.
- Show the current configured adapter.
- When the configuration prevents outbound email from being delivered, show a
warning.
- Detect 'curl' extension during setup since it's more-or-less required
- Add curl extension to the install scripts
codeblock: can you verify the rhel-derivs changes are correct?
Test Plan:
Set adapter to test, verified warning; entered setup mode and verified curl. Ran
apt-get on an ubuntu box. Ran yum on an amazon linux box.
Reviewed By: toulouse
Reviewers: toulouse, codeblock
Commenters: codeblock
CC: aran, jungejason, tuomaspelkonen, codeblock, epriestley, toulouse
Differential Revision: 438
