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

Improve assembly loading time by moving to main thread #1438

Merged
merged 27 commits into from
Nov 14, 2020
Merged

Conversation

cmdcolin
Copy link
Collaborator

This is an alternative to #1406 that performs assembly loading on the main thread

This improves loading time, and makes it so the assembly is waited on before the app is called initialized

@cmdcolin
Copy link
Collaborator Author

Note that the goals of this PR are knock-on effects from wanting to add a test for the location-search box, which means the assembly has to be initialized

Therefore we

  • make LGV displaying conditional on the assembly being loaded (see the LGV initialized getter)
  • and perform assembly loading in the main thread

If assembly loading is not done in the main thread, the loading time for a basic LGV is much worse because it has to wait on the webworker JS to download, parse, run, etc before anything is displayed

@cmdcolin
Copy link
Collaborator Author

The tests on this branch for me pass locally but do not pass on travis https://travis-ci.com/github/GMOD/jbrowse-components/builds/200457135

@cmdcolin cmdcolin marked this pull request as draft November 12, 2020 18:40
@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #1438 (79c3d37) into master (a764c01) will decrease coverage by 0.18%.
The diff coverage is 75.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
- Coverage   60.00%   59.81%   -0.19%     
==========================================
  Files         426      425       -1     
  Lines       19395    19004     -391     
  Branches     4581     4448     -133     
==========================================
- Hits        11638    11368     -270     
+ Misses       7465     7346     -119     
+ Partials      292      290       -2     
Impacted Files Coverage Δ
...kages/core/BaseFeatureWidget/BaseFeatureDetail.tsx 96.66% <ø> (ø)
packages/core/babel.config.js 0.00% <ø> (ø)
packages/core/pluggableElementTypes/ViewType.ts 81.81% <ø> (-12.30%) ⬇️
packages/core/pluggableElementTypes/WidgetType.ts 81.81% <ø> (-6.42%) ⬇️
...pes/renderers/ComparativeServerSideRendererType.ts 86.00% <ø> (-1.84%) ⬇️
packages/core/rpc/coreRpcMethods.ts 84.31% <ø> (-1.20%) ⬇️
packages/core/ui/App.js 63.63% <ø> (ø)
packages/core/ui/StartScreen.tsx 50.00% <ø> (ø)
packages/core/util/types/index.ts 50.00% <ø> (ø)
packages/development-tools/src/build.ts 94.73% <ø> (ø)
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b480a50...c2ebab2. Read the comment docs.

@cmdcolin
Copy link
Collaborator Author

This is now tests passing

@rbuels
Copy link
Contributor

rbuels commented Nov 13, 2020

if CoreGetRegions and CoreGetRefNameAliases are no longer used, let's add deleting them to this PR

@cmdcolin cmdcolin marked this pull request as ready for review November 14, 2020 00:40
@cmdcolin
Copy link
Collaborator Author

I was somewhat comfortable with this

However, running git checkout origin/master test_data....This reverts the test_data/volvox/config.json back to volvox.2bit and then the tests fail...the volvox.2bit fails while the indexedfasta succeeds. This is somewhat concerning and may be worth looking into

@cmdcolin
Copy link
Collaborator Author

Traced the aforementioned loading failure of volvox.2bit to usage of jest.useFakeTimers being enabled (and then deep in http-range-fetcher depending on usage of a setTimeout). I disabled usage of useFakeTimers for now to fix

@cmdcolin cmdcolin merged commit 95c5f56 into master Nov 14, 2020
@cmdcolin cmdcolin deleted the search_test3 branch November 14, 2020 06:13
@garrettjstevens garrettjstevens added the enhancement New feature or request label Nov 23, 2020
@cmdcolin cmdcolin changed the title Perform assembly loading on the main thread Perform assembly loading on the main thread to avoid webworker initialization, and add integration test for the search box Nov 24, 2020
@cmdcolin cmdcolin changed the title Perform assembly loading on the main thread to avoid webworker initialization, and add integration test for the search box Improve assembly loading time by moving to main thread and create integration test for the LGV locstring navigation box Nov 24, 2020
@cmdcolin cmdcolin changed the title Improve assembly loading time by moving to main thread and create integration test for the LGV locstring navigation box Improve assembly loading time by moving to main thread Nov 24, 2020
@cmdcolin cmdcolin mentioned this pull request Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants