Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Don't sort sites when adding or removing them #9432

Merged
merged 1 commit into from
Jun 14, 2017

Conversation

ayumi
Copy link
Contributor

@ayumi ayumi commented Jun 13, 2017

  • sites is an OrderedMap sorted initially when the app starts.
  • addSite sets site order to sites.size or retains the original order – in both cases iteration order is preserved.
  • removeSite removes a site, also preserving iteration order.

Fix #9427

Auditors: @darkdh @bsclifton

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Because both operations retain sites iteration order.

Fix #9427
@ayumi ayumi added the perf label Jun 13, 2017
@ayumi ayumi self-assigned this Jun 13, 2017
@ayumi ayumi requested review from bsclifton and darkdh June 13, 2017 23:14
Copy link
Member

@darkdh darkdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. let's wait for the test before merging it

@ayumi
Copy link
Contributor Author

ayumi commented Jun 14, 2017

no new failing tests

@darkdh
Copy link
Member

darkdh commented Jun 14, 2017

cc @bsclifton

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticeable improvement when visiting sites. I have maybe 10,000 bookmarks in my test account and it's working great. I went back and compared against master to verify the speed increases.

Great job 😄 👍

@bsclifton
Copy link
Member

I think this makes sense to pull into 0.17.x. What do you think, @bbondy?

@bsclifton bsclifton added this to the 0.17.x (Beta Channel) milestone Jun 14, 2017
@bsclifton bsclifton merged commit 89058cf into master Jun 14, 2017
@bsclifton bsclifton deleted the fix/over-sorting-sites branch June 14, 2017 05:38
bsclifton added a commit that referenced this pull request Jun 14, 2017
Don't sort sites when adding or removing them
bsclifton added a commit that referenced this pull request Jun 14, 2017
Don't sort sites when adding or removing them
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants