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.

Sign up for GitHub

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

Jump to bottom

fix: Replace initEval binding success event #14662

Merged
merged 4 commits into from
Jun 24, 2022
Merged

fix: Replace initEval binding success event #14662

merged 4 commits into from
Jun 24, 2022

Conversation

rishabhrathod01
Copy link
Contributor

@rishabhrathod01 rishabhrathod01 commented Jun 18, 2022 •
edited
Loading

Description

We remove logging successful binding for 1st evaluation as we don't aim to capture events for bindings not added by user in this case.

Fixes #14642

Type of change

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

How Has This Been Tested?

Checklist:

  • 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

@vercel
Copy link

vercel bot commented Jun 18, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Jun 22, 2022 at 2:32PM (UTC)

@github-actions github-actions bot added the Bug Something isn't working label Jun 18, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@rishabhrathod01 rishabhrathod01 linked an issue Jun 18, 2022 that may be closed by this pull request
[Bug]: User Application crash with out of memory error #14642
Closed
1 task
@github-actions github-actions bot added Needs More Info Needs additional information Needs Triaging Needs attention from maintainers to triage Performance Pod All things related to Appsmith performance labels Jun 18, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@rishabhrathod01
Copy link
Contributor Author

Using segment analytics batching of API seems better solution here.

@rishabhrathod01 rishabhrathod01 added the Javascript Product Issues related to users writing javascript in appsmith label Jun 21, 2022
@github-actions github-actions bot added Widgets Product This label groups issues related to widgets High This issue blocks a user from building or impacts a lot of users UI Building Product Issues related to the UI Building experience UI Building Pod labels Jun 21, 2022
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@rishabhrathod01 rishabhrathod01 self-assigned this Jun 21, 2022
@rishabhrathod01 rishabhrathod01 marked this pull request as ready for review June 21, 2022 09:55
@rishabhrathod01
Copy link
Contributor Author

/ok-to-test sha=3a0ceb3

@rishabhrathod01
Copy link
Contributor Author

/ok-to-test sha=3a0ceb3

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2536852853.
Workflow: Appsmith External Integration Test Workflow.
Commit: 3a0ceb3.
PR: 14662.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

2 similar comments
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

1 similar comment
@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@github-actions
Copy link

UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2536852853.
Commit: 3a0ceb3.
Results:

Click to view performance test results

Run 1 Run 2 Run 3 Run 4 Run 5 Median Mean SD.Sample SD.Population
SELECT_CATEGORY
scripting 761.88 1053.38 741.92 1938.61 844.95 844.95 1068.15 47.00 42.03
painting 4.54 7.75 4.24 4.99 5.87 4.99 5.48 25.73 22.99
rendering 401.8 520.92 409.04 471.13 426.42 426.42 445.86 11.19 10.00
BIND_TABLE_DATA
scripting 2179.84 2035.69 2061.95 2177.35 2193.43 2177.35 2129.65 3.50 3.13
painting 21.98 15.75 20.48 25.75 16.21 20.48 20.03 20.82 18.62
rendering 973.76 1302.04 1164.52 1302.36 926.42 1164.52 1133.82 15.67 14.02
CLICK_ON_TABLE_ROW
scripting 1859.11 1979.04 3709.09 2798.15 2221.36 2221.36 2513.35 30.23 27.04
painting 12.14 23.59 49.56 40.3 25 25 30.12 49.07 43.89
rendering 360.32 423.07 900.88 653.38 397.21 423.07 546.97 41.81 37.40
UPDATE_POST_TITLE
scripting 3288.35 4277.11 4528.44 3869.16 3690.51 3869.16 3930.71 12.41 11.10
painting 21.87 21.85 27.02 23.02 28.62 23.02 24.48 12.83 11.48
rendering 521.98 565.3 730.41 643.01 588.66 588.66 609.87 13.17 11.78
OPEN_MODAL
scripting 1455.14 3466.92 3224.3 1478.38 1362.18 1478.38 2197.38 47.90 42.84
painting 17.08 14.52 16.4 20.3 11.45 16.4 15.95 20.50 18.31
rendering 635.09 732.7 772.05 715.73 606.2 715.73 692.35 10.01 8.96
CLOSE_MODAL
scripting 655.98 875.29 824.81 792.4 1094.27 824.81 848.55 18.81 16.82
painting 5.17 6.67 9.78 11.65 8.59 8.59 8.37 30.47 27.24
rendering 480.73 618.89 594.22 578.1 659.18 594.22 586.22 11.33 10.13
SELECT_WIDGET_MENU_OPEN
scripting 1542.71 1545.81 1622.1 1590.11 1784.87 1590.11 1617.12 6.15 5.50
painting 14.99 10.74 13.64 12.93 14.4 13.64 13.34 12.37 11.02
rendering 886.03 833.72 1002.82 884.86 1012.25 886.03 923.94 8.58 7.67
SELECT_WIDGET_SELECT_OPTION
scripting 325.21 265.96 216.12 251.98 253.65 253.65 262.58 15.10 13.50
painting 4.34 11.29 7.19 5.13 8.37 7.19 7.26 38.02 34.02
rendering 17.62 19.77 18.73 20.49 17.68 18.73 18.86 6.73 5.99

@rishabhrathod01
Copy link
Contributor Author

rishabhrathod01 commented Jun 21, 2022 •
edited
Loading

Ideally, we should be tracking when user add dynamicBinding is successfully binded.

To do that we could track dynamicBindingPathsList diff for newly added bindings.
Screenshot 2022-06-22 at 1 19 24 AM

Current dynamicBindingPathsList structure is not best for finding this diff easily. Hence, to resolve above issue currently we only stop this events for first evaluation. Solving user crash issue.

rimildeyjsr
rimildeyjsr approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@rimildeyjsr rimildeyjsr left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

Unable to find test scripts. Please add necessary tests to the PR.

@ramsaptami
Copy link
Contributor

Tested user's app on DP and the issue is not reproducible anymore

@rishabhrathod01 rishabhrathod01 merged commit 5d1e6b8 into release Jun 24, 2022
@rishabhrathod01 rishabhrathod01 deleted the fix/replace-initEvalEvents branch June 24, 2022 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rimildeyjsr rimildeyjsr rimildeyjsr approved these changes

@ohansFavour ohansFavour Awaiting requested review from ohansFavour

Assignees

@rishabhrathod01 rishabhrathod01

Labels
Bug Something isn't working High This issue blocks a user from building or impacts a lot of users Javascript Product Issues related to users writing javascript in appsmith Needs More Info Needs additional information Needs Triaging Needs attention from maintainers to triage Performance Pod All things related to Appsmith performance UI Building Product Issues related to the UI Building experience Widgets Product This label groups issues related to widgets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: User Application crash with out of memory error
3 participants
@rishabhrathod01 @ramsaptami @rimildeyjsr