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

reduce getSiteDataFromRecord time from ~300ms to ~2ms #10281

Merged
merged 1 commit into from
Aug 4, 2017
Merged

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Aug 3, 2017

Fix #10273 by minimizing the number of immutableJS to/from native object conversions. The bulk of the performance gain is from removing the line record = Immutable.fromJS(record).

Test Plan:

  1. import 6000 bookmarks and wait for them to upload in pyramid 0
  2. open pyramid 1, sync to pyramid 0. bookmarks should import within a couple seconds instead of never.

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

@diracdeltas diracdeltas added hackathon Legacy label for a hackaton. perf labels Aug 3, 2017
@diracdeltas diracdeltas self-assigned this Aug 3, 2017
@diracdeltas diracdeltas requested a review from ayumi August 3, 2017 21:00
@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

Merging #10281 into master will decrease coverage by 0.02%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master   #10281      +/-   ##
==========================================
- Coverage   53.42%    53.4%   -0.03%     
==========================================
  Files         238      238              
  Lines       21056    21069      +13     
  Branches     3253     3255       +2     
==========================================
+ Hits        11249    11251       +2     
- Misses       9807     9818      +11
Flag Coverage Δ
#unittest 53.4% <40%> (-0.03%) ⬇️
Impacted Files Coverage Δ
js/state/syncUtil.js 39.76% <40%> (-0.64%) ⬇️

@diracdeltas diracdeltas added this to the 0.20.x (Developer Channel) milestone Aug 3, 2017
@diracdeltas
Copy link
Member Author

going to move to 0.21.x so to avoid having to do the work for this PR twice (once for pre-sites-split, one for after)

@diracdeltas diracdeltas modified the milestones: 0.21.x (Nightly Channel), 0.20.x (Developer Channel) Aug 4, 2017
@diracdeltas
Copy link
Member Author

getting an error upon uploading bookmarks after the rebase:

Error: objectId must be an Array
    at getObjectById (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:453:11)
    at getFolderIdByObjectId (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:491:17)
    at getBookmarkSiteDataFromRecord (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:127:7)
    at Object.module.exports.getSiteDataFromRecord (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:169:17)
    at records.forEach (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:309:39)
    at Array.forEach (<anonymous>)
    at Object.module.exports.applySyncRecords (/Users/yan/repos/browser-laptop/js/state/syncUtil.js:304:11)
    at EventEmitter.ipcMain.on (/Users/yan/repos/browser-laptop/app/sync.js:524:14)
    at emitThree (events.js:116:13)
    at EventEmitter.emit (events.js:197:7)

@diracdeltas diracdeltas merged commit 532a7ce into master Aug 4, 2017
@bsclifton bsclifton deleted the fix/10273 branch August 30, 2017 19:30
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hackathon Legacy label for a hackaton. perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants