Page MenuHomePhabricator

T603 takes 2 seconds to render (DarkConsole)
Closed, DuplicatePublic

Description

Very large tasks with many comments and attachments can take a long time to render vs. our usual load times. See T603 for an example.

Event Timeline

joshuaspence assigned this task to epriestley.
joshuaspence raised the priority of this task from to Needs Triage.
joshuaspence updated the task description. (Show Details)
joshuaspence added projects: Mobile, Phabricator.
joshuaspence added a subscriber: joshuaspence.
chad added a subscriber: chad.Jul 8 2014, 8:08 PM

Can we make this task a little more specific/actionable? Some things are going to be slow that are not the specific fault of Phabricator and I'm not sure it's worth our time to fix edge cases like T603.

chad added a comment.Jul 8 2014, 8:09 PM

Mobile rendering improved about 3-4x with the redesign and rollout of FontAwesome. There aren't many other savings left.

chad renamed this task from Sluggish on mobile to T603 takes 2 seconds to render (DarkConsole).Jul 8 2014, 8:36 PM
chad triaged this task as Normal priority.
chad updated the task description. (Show Details)
chad removed a project: Mobile.
chad added a comment.Jul 8 2014, 8:38 PM

I'm going to make this about T603, specifically, since it'd obviously slow. As far as mobile speed, I'd prefer to have specific actionable items that make all of mobile faster. It should have gotten significantly better in the past month, but we can take a second look to make sure there isn't any other low hanging fruit.

I'm not familiar with the reasoning as to why it is slow (figured that you guys might), but I am more concerned with the performance on large diffs/commits (unfortunately we have a lot of these). For some reason, the performance issues seem worse on mobile.

chad added a comment.Jul 8 2014, 10:15 PM

Differential on mobile is sorta half baked. We turned it on since it was more useful than it's current state, but not the ideal state. T2009 I think is the main piece we're waiting on (1-up views). Though I can't seem to find the specific task I thought we had on it (1-up views for mobile).

My guess is that T603 is slow because it has 350+ comments. These are in the DOM, just hidden with CSS.

Also, what phone/browser? T603 isn't bad for me on my ancient (2011) iPhone with Safari. It takes 3-4 seconds to load but the page scrolls/works fine once it loads. However, for me, 347 of the 350+ comments are hidden because I took an action in the transaction log.

T4712 covers the general transaction pagination issue: for very long threads, we should never load/show more than the most recent ~50 transactions, and should do some kind of "this is super noisy, click here to load 100 more older transactions" thing.

For diffs/commits, we could maybe lower some of the thresholds if we detect a mobile UA (for example, hide content by default at 20 files instead of 100), but I don't think we should put a ton of work into optimizing this experience since I think it's just never going to be very pleasant to review a large amount of code on a phone. A real 1-up view should also make this stuff more usable, as @chad mentions.

I'm just going to merge this into T4712 -- from the profile, the server side time (~2-3 seconds of that ~3-4 seconds) is almost entirely spent rendering the hundreds of transactions. While we could probably make this faster, we have to cut it off somewhere: if a task has 10,000 or 100,000 transactions, it's clearly pointless to show them all by default, and we'll run into situations like this sooner or later.

For revisions and commits specifically, let's do this:

  • Wait for T2009 first (working 1-up, default to 1-up on mobile)
  • If performance/experience is routinely problematic after that, let's try reducing thresholds for "large diffs".
  • If that's not good enough and people are actually using it, we can build some kind of "swipe to review" thing maybe: scroll up/down to view the file, swipe left/right to change files, only one file in the DOM at a time, some preloading. That's probably not tons of work or complexity.
  • If that's still not good enough we can look at it some more or start a petition for a native app or something, but I think at some point we're just going to hit hardware limitations, and the ultimate right answer is "don't review giant code changes on your phone".
epriestley closed this task as a duplicate.Jul 9 2014, 1:14 AM

✘ Merged into T4712.

Thanks @epriestley. I'm on a fairly recent phone (Nexus 5), but having viewed T603 again just now and it seems much better... Could've been a network issue on my part (I'm international at the moment).

Anyway, +1 on having a threshold. It doesn't make sense to load an arbitrary number of transactions.

As far as diffs/commits are concerned... the current performance issues aren't limited to mobile. As mentioned on IRC, I managed to peak at 100% usage on our host just by browsing a few (20-30) large commits in differential. This is moreso pygmentize than anything else, but still something that needs to be addressed.

Perhaps it would be reasonable to develop some additional lexers that could be used for syntax highlighting without invoking pygmentize. We should be able to create a JSON lexer fairly easily (based off of PhutilJSONParser). Not so sure about other syntaxes (CSS, HTML, JS would likely be the big ones for us)

chad added a comment.Jul 10 2014, 4:33 PM

I am suspicious of NZ mobile intertubes too. ;)