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

fix: validate dialog take a long time when updating lg in form #5483

Merged
merged 20 commits into from
Jan 13, 2021

Conversation

lei9444
Copy link
Contributor

@lei9444 lei9444 commented Jan 11, 2021

Description

Root cause
The lg file update will trigger the validation function for all dialogs. A single dialog will take about 15ms, and if we have 10 dialogs in one bot, the validate time maybe 150ms.

Fix

  1. use atomFamily to replace the lgFilesState. So the dialog will only listen to the updating lg file and other dialogs will do nothing.
  2. Separate the dialogs' diagnostics from the dialogState.
  3. add cache for the validateDialog function. Improve the time from 15ms to 8ms.

Task Item

#minor

Screenshots

Main branch:
image

This PR
image

@boydc2014
Copy link
Contributor

Looks good, this should be the last piece to include for a patch release.

@srinaath srinaath self-assigned this Jan 11, 2021
Copy link
Contributor

@srinaath srinaath left a comment

Choose a reason for hiding this comment

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

@lei9444 try adding a remote skill to composer. It seems to break the app inside the validateDialogs

@coveralls
Copy link

coveralls commented Jan 12, 2021

Coverage Status

Coverage increased (+0.03%) to 54.888% when pulling 472f593 on lei9444:asyncfun into 55c9054 on microsoft:main.

@lei9444
Copy link
Contributor Author

lei9444 commented Jan 12, 2021

@lei9444 try adding a remote skill to composer. It seems to break the app inside the validateDialogs

Thanks @srinaath I have fixed the issue

@boydc2014 boydc2014 merged commit b136973 into microsoft:main Jan 13, 2021

updateLgFiles(callbackHelpers, projectId, { updates: updatedFiles }, (current, changed) => {
// compare to drop expired content already setted above.
return current?.content === changed?.content;
Copy link
Contributor

@zhixzhan zhixzhan Jan 29, 2021

Choose a reason for hiding this comment

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

This line break multilang file sync, We need also compare id, because "updateFiles" can be other lg file, and the content is always different , then will always drop their changes. Fixed this in #5654

@lei9444 lei9444 deleted the asyncfun branch February 1, 2021 02:06
lei9444 added a commit to lei9444/BotFramework-Composer-1 that referenced this pull request Jun 15, 2021
…soft#5483)

* async selector

* update lg files state

* update the luProvider for dialog

* fix unit test

* add cache for dialog validate

* fix lint

* fix comments

* fix tests

* fix lint

* update text

* fix lint

* fix lint

Co-authored-by: Srinaath Ravichandran <srinaath27@gmail.com>
Co-authored-by: Dong Lei <donglei@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants