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

add node fiber to improve performance #2319

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

abbyhu2000
Copy link
Member

@abbyhu2000 abbyhu2000 commented Sep 12, 2022

Description

For issue #2223, in order to solve the slow compilation time that is caused by changing from node-sass to dart-sass, one possible solution is to add a node-fibers package. Pulling from the sass official website, "to avoid this performance hit, render() can use the fibers package to call asynchronous importers from the synchronous code path". And i have validated the performance improvement: the first compilation changed to 76s from the previous 409s.

However, node-fiber package has reached end of life with Node 16. If the performance issue is a problem, we can consider stay on Node 14 and use node-fiber for now, and remove fibers when migrating to Node 16 later.

Issues Resolved

resolves #2223

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@abbyhu2000 abbyhu2000 marked this pull request as ready for review September 12, 2022 17:44
@abbyhu2000 abbyhu2000 requested a review from a team as a code owner September 12, 2022 17:44
@abbyhu2000 abbyhu2000 self-assigned this Sep 12, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2022

Codecov Report

Merging #2319 (1e0c289) into main (77af7f9) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 1e0c289 differs from pull request most recent head 378ece2. Consider uploading reports for the commit 378ece2 to get more accurate results

@@            Coverage Diff             @@
##             main    #2319      +/-   ##
==========================================
- Coverage   66.55%   66.55%   -0.01%     
==========================================
  Files        3169     3169              
  Lines       60313    60313              
  Branches     9182     9182              
==========================================
- Hits        40141    40139       -2     
- Misses      17979    17981       +2     
  Partials     2193     2193              
Impacted Files Coverage Δ
...s/osd-optimizer/src/node/node_auto_tranpilation.ts 83.67% <0.00%> (-4.09%) ⬇️
packages/osd-optimizer/src/node/cache.ts 50.00% <0.00%> (-2.78%) ⬇️
...ic/application/models/sense_editor/sense_editor.ts 64.88% <0.00%> (+0.88%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ashwin-pc
Copy link
Member

@abbyhu2000 can you sign off the commit to pass DCO?

@abbyhu2000 abbyhu2000 force-pushed the sass_experiment branch 2 times, most recently from 1e0c289 to 378ece2 Compare September 12, 2022 19:53
@AMoo-Miki
Copy link
Collaborator

Abby, did you get a chance to experiment with sass-embedded?

@abbyhu2000
Copy link
Member Author

abbyhu2000 commented Sep 12, 2022

Abby, did you get a chance to experiment with sass-embedded?

created a follow up issue for researching on sass-embedded: #2331 and it shows using sass-embedded does not really shorten the first compilation time.

@kavilla kavilla merged commit 9f7ba5b into opensearch-project:main Sep 13, 2022
@kavilla kavilla added build Build related additions or modifications v3.0.0 labels Sep 13, 2022
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Sep 14, 2022
For issue [opensearch-project#2223](opensearch-project#2223), in order to solve the slow compilation time that is caused by changing from node-sass to dart-sass, one possible solution is to add a node-fibers package. Pulling from the sass official website, "to avoid this performance hit, render() can use the fibers package to call asynchronous importers from the synchronous code path". And i have validated the performance improvement: the first compilation changed to 76s from the previous 409s.

However, node-fiber package has reached end of life with Node 16. If the performance issue is a problem, we can consider stay on Node 14 and use node-fiber for now, and remove fibers when migrating to Node 16 later.

Issue resolved:
opensearch-project#2223

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
sipopo pushed a commit to sipopo/OpenSearch-Dashboards that referenced this pull request Dec 16, 2022
For issue [opensearch-project#2223](opensearch-project#2223), in order to solve the slow compilation time that is caused by changing from node-sass to dart-sass, one possible solution is to add a node-fibers package. Pulling from the sass official website, "to avoid this performance hit, render() can use the fibers package to call asynchronous importers from the synchronous code path". And i have validated the performance improvement: the first compilation changed to 76s from the previous 409s.

However, node-fiber package has reached end of life with Node 16. If the performance issue is a problem, we can consider stay on Node 14 and use node-fiber for now, and remove fibers when migrating to Node 16 later.

Issue resolved:
opensearch-project#2223

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: Sergey V. Osipov <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Bump node.js to 18 and fix errors

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 14, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 15, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 15, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 15, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Mar 21, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>

Fix async unit test timeout issue

Signed-off-by: Anan Zhuang <[email protected]>

[Nodejs 18] fix lmdb and plugins discovery unit tests

Signed-off-by: Anan Zhuang <[email protected]>

Fix windows path

Signed-off-by: Anan Zhuang <[email protected]>

Increase memory limit for unit test and fix memory leak

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 16, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
kavilla pushed a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Apr 18, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>

add unhandle-rejections

Signed-off-by: Anan Zhuang <[email protected]>

add worker

add mock lmdb to integration test

Signed-off-by: Anan Zhuang <[email protected]>

modify test start opensearch

Signed-off-by: Anan Zhuang <[email protected]>

only one integration test

Signed-off-by: Anan Zhuang <[email protected]>

update test

Signed-off-by: Anan Zhuang <[email protected]>

increase time

Signed-off-by: Anan Zhuang <[email protected]>
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 18, 2023
Revert "add node fiber to improve performance (opensearch-project#2319)"
Revert "[CVE-2022-25758] Use dart-sass instead of node-sass (opensearch-project#2054)"
Revert back to use node-sass and bump to 8.0.0
Change lmdb-store to lmdb
Bump node.js to 18 and fix errors

Issue Resolved:
opensearch-project#3601

Signed-off-by: Anan Zhuang <[email protected]>
@abbyhu2000 abbyhu2000 deleted the sass_experiment branch June 30, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build related additions or modifications v3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] osd/optimizer takes too long(~5 min) to respond
5 participants