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

Improvements for the startup time (Bloodhound) #10738

Merged
merged 2 commits into from
Sep 2, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Aug 31, 2017

Please don't squash commits

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.

Based on this testings #3025 (comment) (check section from 30.08.2017) with this fix startup time was reduced from ~12s to ~8.5s.

1 commit -> initial add is split into multiple chunks

2 commit -> bloodhound is using immutable

Resolves #10723

Auditors:

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

@NejcZdovc NejcZdovc added the perf label Aug 31, 2017
@NejcZdovc NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Aug 31, 2017
@NejcZdovc NejcZdovc self-assigned this Aug 31, 2017
@codecov-io
Copy link

codecov-io commented Aug 31, 2017

Codecov Report

Merging #10738 into master will increase coverage by <.01%.
The diff coverage is 87.3%.

@@            Coverage Diff             @@
##           master   #10738      +/-   ##
==========================================
+ Coverage    54.1%   54.11%   +<.01%     
==========================================
  Files         247      247              
  Lines       21519    21532      +13     
  Branches     3332     3331       -1     
==========================================
+ Hits        11643    11652       +9     
- Misses       9876     9880       +4
Flag Coverage Δ
#unittest 54.11% <87.3%> (ø) ⬆️
Impacted Files Coverage Δ
app/browser/reducers/urlBarSuggestionsReducer.js 77.14% <66.66%> (ø) ⬆️
app/common/lib/siteSuggestions.js 89.92% <83.33%> (-2.88%) ⬇️
app/common/lib/suggestion.js 68.35% <95.83%> (-0.13%) ⬇️

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Aug 31, 2017

(note that this times can vary based on computer and computer usage)

first commit improvement

before chunks
init: 15349.250ms
all: 15563.696ms

init: 12331.356ms
all: 12503.437ms

init: 10831.314ms
all: 11056.482ms

init: 10927.020ms
all: 11115.599ms

init: 12617.369ms
all: 12800.794ms

after chunks
init: 8802.901ms
all: 9081.717ms

init: 8631.733ms
all: 8845.051ms

init: 8809.358ms
all: 9146.503ms

init: 8555.203ms
all: 8772.487ms

init: 8622.259ms
all: 8922.692ms

second commit improvement

before immutable usage
init: 8802.901ms
all: 9081.717ms

init: 8631.733ms
all: 8845.051ms

init: 8809.358ms
all: 9146.503ms

init: 8555.203ms
all: 8772.487ms

init: 8622.259ms
all: 8922.692ms

after immutable usage
init: 8026.192ms
all: 8277.633ms

init: 8310.651ms
all: 8534.002ms

init: 7965.882ms
all: 8169.939ms

init: 7875.339ms
all: 8095.693ms

init: 7754.056ms
all: 7960.220ms

@NejcZdovc NejcZdovc changed the title Split bloodhounds initial add into chunks Improvements for the startup time (Bloodhound) Aug 31, 2017
@NejcZdovc NejcZdovc force-pushed the hotfix/#10723-bloodhound branch 4 times, most recently from f39fc80 to d227a1d Compare September 1, 2017 13:59
@NejcZdovc NejcZdovc force-pushed the hotfix/#10723-bloodhound branch from d227a1d to 6daaf36 Compare September 1, 2017 16:12
@NejcZdovc NejcZdovc requested review from bbondy and bsclifton and removed request for bbondy September 1, 2017 16:39
@bbondy bbondy merged commit 302b414 into brave:master Sep 2, 2017
parsedHistorySites.push(
urlParse(site.location)
urlParse(site.get('location'))
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: this might be better like (to avoid calling get twice):

const siteLocation = site.get && site.get('location')
if (siteLocation) {
  parsedHistorySites.push(
    urlParse(siteLocation)
  )
}

let host1 = s1.parsedUrl.host || s1.parsedUrl.pathname || s1.location || ''
let host2 = s2.parsedUrl.host || s2.parsedUrl.pathname || s2.location || ''
let host1 = s1.get('parsedUrl').host || s1.get('parsedUrl').pathname || s1.get('location') || ''
let host2 = s2.get('parsedUrl').host || s2.get('parsedUrl').pathname || s2.get('location') || ''
Copy link
Member

@bsclifton bsclifton Sep 4, 2017

Choose a reason for hiding this comment

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

maybe I'm missing something- shouldn't this be like:
s1.getIn(['parsedUrl', 'host']) / s1.getIn(['parsedUrl', 'pathname']) ?

Copy link
Member

Choose a reason for hiding this comment

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

(same with the changes below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logic was not changed with this PR, so cc @bbondy for this thoughts

Copy link
Member

Choose a reason for hiding this comment

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

@NejcZdovc it was changed from mutable to immutable though... which means you have to use get in order to read the values. Trying to access like a mutable usually returns undefined

Copy link
Contributor Author

@NejcZdovc NejcZdovc Sep 4, 2017

Choose a reason for hiding this comment

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

@bsclifton I see what you mean. Let me double check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsclifton yeah it's working correctly. You can have regular object in immutable object

image

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bloodhound improvements for start up time
4 participants