Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$1000] [Performance] Pressing the FAB on the start screen doesn't open the menu immediately #12119

Closed
5 tasks done
hannojg opened this issue Oct 25, 2022 · 25 comments
Closed
5 tasks done
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review

Comments

@hannojg
Copy link
Contributor

hannojg commented Oct 25, 2022

What performance issue do we need to solve?

When pressing the FAB on the start screen there is a noticeable delay until the menu opens. It gets more noticeable the weaker the device is.

See this video:

before.mp4

What is the impact of this on end-users?

Frustration. The action that follows an interaction shouldn't take any longer than 100 to max 200ms. Otherwise, the user will notice the wait time and gets a poor sense of quality.

List any benchmarks that show the severity of the issue

  • Time from interaction to action: ~2 seconds (From the video above)
  • Worst UI Thread FPS: 55.7
  • Worst JS Thread FPS: 0
  • Time the JS thread froze: ~ 1 seconds

Proposed solution (if any)

In general, we want to avoid doing unnecessary work like rerendering components that don't have to.

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

Note: These should be the same as the benchmarks collected before any changes.

after.mp4

The feedback comes immediately after pressing the FAB. And as there is no frozen JS thread we also can see the animation of the FAB.

  • Time from interaction to action: 0 s (there is probably some, but it's not "eye measurable")
  • Worst UI Thread FPS: 56.2
  • Worst JS Thread FPS: 46.5
  • Time the JS thread froze: 0 ms

Screenshot 2022-10-28 at 15 58 36

Screenshot 2022-10-28 at 15 58 43

(You can see that the JS thread is very less busy, which is a good improvement!)

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.19-1
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

Upwork Automation - Do Not Edit

@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2022

Triggered auto assignment to @AndrewGable (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2022

📣 @hannojg! 📣
Please report bugs or suggest features in the #expensify-open-source Slack channel, don't directly open issues in this repo!
Instructions here to join the channel 📖

@hannojg
Copy link
Contributor Author

hannojg commented Oct 28, 2022

PR is ready for review

@mountiny mountiny added the Reviewing Has a PR in review label Oct 28, 2022
@melvin-bot

This comment was marked as off-topic.

@melvin-bot
Copy link

melvin-bot bot commented Oct 31, 2022

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@mountiny
Copy link
Contributor

This one might be on topic

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Nov 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 21, 2022

This issue has not been updated in over 15 days. @AndrewGable, @hannojg eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@hannojg
Copy link
Contributor Author

hannojg commented Nov 21, 2022

As #12369 got merged this issue can be closed (or it can be tested and verified that the issue got fixed, not sure what's the procedure is here)

@mountiny
Copy link
Contributor

This will be automatically updated once the PR hits production.

@Santhosh-Sellavel
Copy link
Collaborator

@mountiny Can you assign me this one to track payment for PR review, thanks!

@mountiny mountiny added the Bug Something is broken. Auto assigns a BugZero manager. label Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Nov 30, 2022
@mountiny
Copy link
Contributor

This has hit production one week ago so we can pay @Santhosh-Sellavel for reviewing internal PR, this was bit more involved. What do you suggest is a fair compensation?

cc @alexpensify

@alexpensify
Copy link
Contributor

@mountiny - I've been OOO since November 19. Do you need me to start a convo about payment or start a job here? Thanks!

@mountiny
Copy link
Contributor

mountiny commented Dec 1, 2022

@alexpensify I think we dont need and internal convo for the compensation, we can just ask @Santhosh-Sellavel what they think was reasonable compensation for the testing of that PR.

as far as I recall this PR has been quite involved with testing from Santosh side, what do you suggest @Santhosh-Sellavel ?

@mountiny mountiny added the Awaiting Payment Auto-added when associated PR is deployed to production label Dec 1, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@mountiny
Considering the effort on this PR which is quite a lot, I am requesting consider a minimum of $2K and I think it's fair for the work done. If you feel otherwise, I am happy to take the Flat $1000 price which is the new price! And feel free to add bonuses 😀 , thanks!

@mountiny
Copy link
Contributor

mountiny commented Dec 1, 2022

I agree with the $2000, this has been an External PR which went a bit outside of the classic track, but due to the importance of this issue I thin $2000 would be appropriate at least for this issue so as C+ reviewer this makes sense to me

@alexpensify Would you be able to create a job for this? Thank you!

@alexpensify
Copy link
Contributor

Ok, I'll deal with this one tomorrow!

@alexpensify alexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title [Performance] Pressing the FAB on the start screen doesn't open the menu immediately [$1000] [Performance] Pressing the FAB on the start screen doesn't open the menu immediately Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Job added to Upwork: https://www.upwork.com/jobs/~01a90182453cb9e09e

@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Current assignee @Santhosh-Sellavel is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Current assignee @AndrewGable is eligible for the External assigner, not assigning anyone new.

@alexpensify
Copy link
Contributor

I've started the process to pay via Upwork, please accept. Thanks!

@Santhosh-Sellavel
Copy link
Collaborator

Thanks, accepted! Remove help wanted label.

@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 5, 2022
@mountiny
Copy link
Contributor

mountiny commented Dec 5, 2022

Removed, thanks

@alexpensify
Copy link
Contributor

@Santhosh-Sellavel - Payment has been sent via Upwork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

6 participants