Page MenuHomePhabricator
Create Task
Maniphest T5644

Add controls to prevent `pygmentize` from saturating system resources
Closed, ResolvedPublic
Actions

Description

I've had discussions with @epriestley about this before, but I am opening this as a separate maniphest. Basically, pygmentize is super resource-intensive and we don't have many controls in place to prevent pygmentize from saturating available system resources. Currently, by opening a bunch of (approximately 10) large merge commits in separate tabs I am able to DOS our Phabricator install (subsequent requests will hit a proc_open() error) and saturate CPU usage on a c1.xlarge EC2 instance.

Revisions and Commits

rPHU libphutil
D12147rPHU4c496670539c Don't invoke pygmentize to highlight "txt" or "text" files
rP Phabricator
D12141rP0efae2858ecc Don't syntax highlight codebase pattern search results
D12132rP21826ed7b35a Don't highlight very large files by default

Related Objects
Search...

StatusAssignedTask
 ResolvedepriestleyT5644 Add controls to prevent `pygmentize` from saturating system resources
 ResolvedepriestleyT5179 Unearth Differential rendering technical debt
 ResolvedepriestleyT2183 unify "table of contents" views between Differential and Diffusion
Restricted Maniphest Task
 OpenjoshuaspenceT3626 Detection of language for some file types isn't good
Restricted Maniphest Task
 DuplicateNoneT4988 pygments is run even on very large changesets that won't be shown
 ResolvedepriestleyT2009 Refactor DifferentialChangesetParser to support 1-up views and sanity
Restricted Maniphest Task
 ResolvedepriestleyT4045 Store diffs as binary, not UTF-8
 ResolvedepriestleyT3788 Improve encoding management in Diffusion
 WontfixepriestleyT5030 Improve behavior when jumping to inline comments
 ResolvedepriestleyT1460 Add "Done" link to an inline comment
 ResolvedepriestleyT2222 Implement ApplicationTransactions in Differential
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
Restricted Maniphest Task
 ResolvedepriestleyT4481 Differential Update Feedback / Rough Edges / Status
 ResolvedbtrahanT4896 Move Audit to ApplicationTransactions
 ResolvedbtrahanT5745 Unable to post only inline comments to an audit
 ResolvedchadT7737 "Done" button has misleading UI states
 ResolvedNoneT7720 Incorrect "Unsubmitted" state detection for Audit inline comments
 ResolvedangieT7719 Impossible to submit "Done" inline comment state without adding a comment
 OpenNoneT8250 Would like to see all comments and their status in a list
 ResolvedNoneT8130 Keyboard shortcut for "Done"
 ResolvedNoneT8573 All reviewers should be allowed to mark a comment as done
 OpenNoneT2999 When we detect a file as binary, explain why
 OpenNoneT2495 Improve whitespace expansion of tab literals

Event Timeline

joshuaspence created this task.Jul 17 2014, 2:59 AM
joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added projects: Phabricator, Diffusion.
joshuaspence added subscribers: joshuaspence, epriestley.
epriestley claimed this task.Aug 22 2014, 7:27 PM

I have about half a diff for this.

epriestley added a subscriber: sophiebits.Aug 22 2014, 8:08 PM

◀ Merged tasks: T4988.

epriestley added a subtask: T5179: Unearth Differential rendering technical debt.Aug 22 2014, 8:08 PM
joshuaspence triaged this task as High priority.Oct 12 2014, 9:31 PM
chad removed a project: Phabricator.Nov 3 2014, 2:51 AM
joshuaspence added a comment.Feb 11 2015, 10:52 PM

@epriestley, what's your half-diff do?

epriestley added a comment.Feb 11 2015, 11:03 PM

Let me see what I can dig up, I think I accidentally declared branch bankruptcy at one point..

epriestley added a comment.Feb 11 2015, 11:04 PM

Looks like it's more "5% of a diff":

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

joshuaspence claimed this task.Feb 12 2015, 3:19 AM

I might be able to give this a go.

• nswaney added a subscriber: • nswaney.Feb 25 2015, 9:00 PM
epriestley mentioned this in T3626: Detection of language for some file types isn't good.Mar 4 2015, 3:49 PM
devurandom added a subscriber: devurandom.Mar 4 2015, 3:52 PM
epriestley added a project: Differential.Mar 4 2015, 5:17 PM
epriestley moved this task from Backlog to v2 (UI/Mobile) on the Differential board.
epriestley closed subtask T5179: Unearth Differential rendering technical debt as Resolved.Mar 6 2015, 12:31 AM
epriestley mentioned this in T7559: integration with reverse caching proxies such as squid, varnish.Mar 16 2015, 4:05 AM
epriestley claimed this task.Mar 22 2015, 6:43 PM
epriestley added a revision: D12132: Don't highlight very large files by default.Mar 22 2015, 7:45 PM
epriestley added a commit: rP21826ed7b35a: Don't highlight very large files by default.Mar 24 2015, 12:28 PM
epriestley added a revision: D12141: Don't syntax highlight codebase pattern search results.Mar 24 2015, 12:59 PM
epriestley added a commit: rP0efae2858ecc: Don't syntax highlight codebase pattern search results.Mar 24 2015, 7:47 PM
epriestley closed this task as Resolved.Mar 24 2015, 7:52 PM

I've removed the two major sources of needless/expensive highlighting (D12132, D12141). There's a third that will eventually be resolved by T5501 (roughly, if APC settings are whack you'll run setup checks on every page, including setup checks against pygmentize).

At least for now, I don't plan to go further than that.

epriestley added a revision: D12147: Don't invoke pygmentize to highlight "txt" or "text" files.Mar 24 2015, 8:05 PM
epriestley added a commit: rPHU4c496670539c: Don't invoke pygmentize to highlight "txt" or "text" files.Mar 24 2015, 8:09 PM
urzds added a subscriber: urzds.Jul 12 2017, 11:11 AM
Herald added a subscriber: eadler. · View Herald TranscriptJul 12 2017, 11:11 AM
Phabricator · Built by Phacility