Skip to content

fix: performance improvements for js editor#21492

Merged
eco-monk merged 11 commits intoreleasefrom
fix/perf-mark-test
Mar 21, 2023
Merged

fix: performance improvements for js editor#21492
eco-monk merged 11 commits intoreleasefrom
fix/perf-mark-test

Conversation

@eco-monk
Copy link
Copy Markdown
Contributor

@eco-monk eco-monk commented Mar 16, 2023 •
edited
Loading

Description

TL;DR performance improvements for js editor

  • fix entityNavigationData generation (to prevent unnecessary component updates)
    • in codeEditor/index.ts (addThisReference was creating a new object everytime)
    • in navigationSelector.ts (use getJSCollections instead of getJSCollectionsForCurrentPage, which created a new object everytime, even if actions were not updated)
  • combine markers for navigation and peek overlay to reduce the total number of markers
  • clear and add marks for only the edited lines instead of the whole file

Note: once a js object is saved, it's still going to trigger a whole file clear and marking.
Because, it's an entity update which needs a whole refresh of the markers.

Fixes #21467

Media

Case: Adding a blank space in js editor.

Reduced un-necessary clears and marks:

Before:

image

####After:
image

Reduced entity marker called count:

https://www.loom.com/share/23719f8dfde8457ea0a86f44500ec34a

Reduced markers count:

Before:

image

After:

image

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Manual

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • PR is being merged under a feature flag

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@eco-monk
Copy link
Copy Markdown
Contributor Author

eco-monk commented Mar 16, 2023

/ok-to-test

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 16, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4439136669.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 21492.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=21492&runId=4439136669_1

@eco-monk
Copy link
Copy Markdown
Contributor Author

eco-monk commented Mar 16, 2023

/ok-to-test

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 16, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4439239008.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 21492.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=21492&runId=4439239008_1

@eco-monk eco-monk changed the title Fix/perf mark test fix: improve performance of js editor Mar 17, 2023
@github-actions github-actions bot added the Bug Something isn't working label Mar 17, 2023
@eco-monk eco-monk changed the title fix: improve performance of js editor fix: performance improvements for js editor Mar 17, 2023
@github-actions github-actions bot added Community Reported issues reported by community members High This issue blocks a user from building or impacts a lot of users IDE Navigation Issues/feature requests related to IDE navigation, and context switching Needs Triaging Needs attention from maintainers to triage IDE Pod Issues that new developers face while exploring the IDE Performance Issues related to performance Performance Pod All things related to Appsmith performance Regressed Scenarios that were working before but have now regressed labels Mar 20, 2023
@eco-monk eco-monk mentioned this pull request Mar 20, 2023
Closed
1 task
eco-monk
eco-monk commented Mar 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@eco-monk eco-monk left a comment

Choose a reason for hiding this comment

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

Summary

Comment thread app/client/src/components/editorComponents/CodeEditor/MarkHelpers/bindingMarker.ts
Copy link
Copy Markdown
Contributor Author

@eco-monk eco-monk Mar 20, 2023

Choose a reason for hiding this comment

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

Existing file split up,
No changes here.

Comment thread app/client/src/components/editorComponents/CodeEditor/MarkHelpers/entityMarker.ts
Copy link
Copy Markdown
Contributor Author

@eco-monk eco-monk Mar 20, 2023

Choose a reason for hiding this comment

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

existing file split + changes.
Change commit here.

@eco-monk
Copy link
Copy Markdown
Contributor Author

eco-monk commented Mar 20, 2023

/ok-to-test

@eco-monk
Copy link
Copy Markdown
Contributor Author

eco-monk commented Mar 20, 2023

/build-deploy-preview

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4465273258.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 21492.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=21492&runId=4465273258_1

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2023

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/4465274178.
Workflow: On demand build Docker image and deploy preview.
skip-tests: . env: .
PR: 21492.

hetunandu
hetunandu requested changes Mar 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@hetunandu hetunandu left a comment

Choose a reason for hiding this comment

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

Minor change request

Comment thread app/client/src/selectors/navigationSelectors.ts Outdated
Comment thread app/client/src/selectors/navigationSelectors.ts Outdated
Comment thread app/client/src/components/editorComponents/CodeEditor/index.tsx Outdated
@eco-monk eco-monk requested a review from hetunandu March 20, 2023 06:59
@eco-monk
Copy link
Copy Markdown
Contributor Author

eco-monk commented Mar 20, 2023

/build-deploy-preview env=release

@eco-monk
Copy link
Copy Markdown
Contributor Author

eco-monk commented Mar 20, 2023

/ok-to-test

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2023

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/4465549318.
Workflow: On demand build Docker image and deploy preview.
skip-tests: ``.
env: release.
PR: 21492.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/4465550932.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 21492.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=21492&runId=4465550932_1

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 20, 2023

The following are new failures, please fix them before merging the PR:

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Mar 20, 2023

    Deploy-Preview-URL: https://appsmith-fhn25mls9-get-appsmith.vercel.app

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented Mar 20, 2023

    The following are new failures, please fix them before merging the PR:

    1. cypress/integration/Regression_TestSuite/ClientSideTests/Git/GitWithJSLibrary/GitwithCustomJSLibrary_spec.js

    @eco-monk
    Copy link
    Copy Markdown
    Contributor Author

    eco-monk commented Mar 20, 2023

    Pending QA verification.

    @Richarex
    Copy link
    Copy Markdown

    Richarex commented Mar 21, 2023

    Tested this on the DP url, working as expected.

    @eco-monk eco-monk merged commit 76f2239 into release Mar 21, 2023
    @eco-monk eco-monk deleted the fix/perf-mark-test branch March 21, 2023 05:09
    @eco-monk eco-monk mentioned this pull request Mar 21, 2023
    Merged
    Aishwarya-U-R pushed a commit that referenced this pull request Mar 21, 2023
    @eco-monk
    ci: fix Command_Click_Navigation_spec test (#21634)
    48fb36f 
    ## Description

Fixes `Command_Click_Navigation_spec`

Not sure why the [original
PR](#21492) passed though.
([Test
run](https://github.com/appsmithorg/appsmith/actions/runs/4465550932))
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @hetunandu hetunandu hetunandu approved these changes

    @akash-codemonk akash-codemonk Awaiting requested review from akash-codemonk

    @SatishGandham SatishGandham Awaiting requested review from SatishGandham

    Assignees

    No one assigned

    Labels

    Bug Something isn't working Community Reported issues reported by community members High This issue blocks a user from building or impacts a lot of users IDE Navigation Issues/feature requests related to IDE navigation, and context switching IDE Pod Issues that new developers face while exploring the IDE Needs Triaging Needs attention from maintainers to triage Performance Pod All things related to Appsmith performance Performance Issues related to performance Regressed Scenarios that were working before but have now regressed

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    [Bug]: The app begins to slow down on the cloud version when the app becomes complex

    3 participants

    @eco-monk @Richarex @hetunandu