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

[PORT] Further improve LG's performance #2409

Merged
merged 11 commits into from
Jun 30, 2020
Merged

[PORT] Further improve LG's performance #2409

merged 11 commits into from
Jun 30, 2020

Conversation

Danieladu
Copy link
Contributor

@Danieladu Danieladu commented Jun 24, 2020 •
edited
Loading

issue link: microsoft/botbuilder-dotnet#4156
Fixes #2441
Two operations to improve LG's performance

  1. Remove unnecessary visit operation to reduce the visiting time
  2. main [UpdateTemplate/AddTemplate/DeleteTemplate Method] Separately parse the template that needs to be updated, without parse the whole LG file.

Results comparison table：

LG File CRUD time before refactoring CRUD time after refactoring
file1 with 12630 lines 10000ms 150ms
file2 with 1000 lines 910ms 7ms
file3 with 100 lines 280ms 4ms

@coveralls
Copy link

coveralls commented Jun 24, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 142017

  • 89 of 89 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 80.946%
Totals Coverage Status
Change from base Build 141906: 0.06%
Covered Lines: 12952
Relevant Lines: 15279
💛 - Coveralls

chrimc62
chrimc62 reviewed Jun 26, 2020
View reviewed changes
libraries/botbuilder-lg/src/templates.ts Outdated
@@ -268,20 +270,32 @@ export class Templates implements Iterable<Template> {
*/
public updateTemplate(templateName: string, newTemplateName: string, parameters: string[], templateBody: string): Templates {
const template: Template = this.items.find((u: Template): boolean => u.name === templateName);
if (template === undefined) {
return this;
if (template !== undefined) {
Copy link
Contributor

@chrimc62 chrimc62 Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

template !== undefined [](start = 12, length = 22)

Would if (template) make sense or do you need to do the next steps even if template is an empty string? #Resolved

Copy link
Contributor Author

@Danieladu Danieladu Jun 28, 2020 •
edited by chrimc62
Loading

Choose a reason for hiding this comment

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

#Solved #Resolved

@chrimc62
Copy link
Contributor

chrimc62 commented Jun 26, 2020 •
edited
Loading 

    if (template !== undefined) {

Would if (!template) make sense or is an empty template string a valid template definition? #Resolved

Refers to: libraries/botbuilder-lg/src/templates.ts:311 in 387fc77. [](commit_id = 387fc77, deletion_comment = False)

chrimc62
chrimc62 reviewed Jun 26, 2020
View reviewed changes
libraries/botbuilder-lg/src/templates.ts Outdated

private adjustRangeForUpdateTemplate(oldTemplate: Template, newTemplate: Template): void {
const lineOffset = newTemplate.sourceRange.range.end.line - newTemplate.sourceRange.range.start.line
- (oldTemplate.sourceRange.range.end.line - oldTemplate.sourceRange.range.start.line);
Copy link
Contributor

@chrimc62 chrimc62 Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This would be clearer if you did a const newRange = newTemplate.sourceRange.range and const oldRange = oldTemplate.soureceRange first and rewrote that in terms of this. #Resolved

Copy link
Contributor Author

@Danieladu Danieladu Jun 28, 2020 •
edited by chrimc62
Loading

Choose a reason for hiding this comment

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

#Solved #Resolved

chrimc62
chrimc62 reviewed Jun 26, 2020
View reviewed changes
libraries/botbuilder-lg/src/templatesParser.ts
@@ -175,6 +175,22 @@ export class TemplatesParser {
return templates;
}

public static antlrParseTemplates(text: string, source: string): FileContext {
if (!text || text.trim() === '') {
Copy link
Contributor

@chrimc62 chrimc62 Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Missing documentation of public element. This needs to be public? #Resolved

Copy link
Contributor Author

@Danieladu Danieladu Jun 28, 2020 •
edited by chrimc62
Loading

Choose a reason for hiding this comment

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

#Solved #Resolved

chrimc62
chrimc62 reviewed Jun 26, 2020
View reviewed changes
libraries/botbuilder-lg/src/templatesParser.ts
}


class TemplatesTransformer extends AbstractParseTreeVisitor<any> implements LGTemplateParserVisitor<any> {
export class TemplatesTransformer extends AbstractParseTreeVisitor<any> implements LGTemplateParserVisitor<any> {
Copy link
Contributor

@chrimc62 chrimc62 Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

export [](start = 0, length = 6)

If exporting you should have documentation. #Resolved

Copy link
Contributor Author

@Danieladu Danieladu Jun 28, 2020 •
edited by chrimc62
Loading

Choose a reason for hiding this comment

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

#Solved #Resolved

chrimc62
chrimc62 suggested changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

🕐

@Danieladu Danieladu requested a review from chrimc62 June 28, 2020 01:57
chrimc62
chrimc62 reviewed Jun 29, 2020
View reviewed changes
libraries/botbuilder-lg/src/templatesParser.ts Outdated
@@ -175,6 +175,28 @@ export class TemplatesParser {
return templates;
}

/**
* Parse LG content and achieve the AST.
Copy link
Contributor

Choose a reason for hiding this comment

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

achieve [](start = 28, length = 7)

return

chrimc62
chrimc62 reviewed Jun 29, 2020
View reviewed changes
libraries/botbuilder-lg/src/templatesParser.ts Outdated

class TemplatesTransformer extends AbstractParseTreeVisitor<any> implements LGTemplateParserVisitor<any> {
/**
* Templates transfeormer. Fullfill more details and body context into the templates object.
Copy link
Contributor

@chrimc62 chrimc62 Jun 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

transfeormer [](start = 13, length = 12)

transformer

chrimc62
chrimc62 reviewed Jun 29, 2020
View reviewed changes
libraries/botbuilder-lg/src/templatesParser.ts Outdated

class TemplatesTransformer extends AbstractParseTreeVisitor<any> implements LGTemplateParserVisitor<any> {
/**
* Templates transfeormer. Fullfill more details and body context into the templates object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fullfill [](start = 27, length = 8)

Add

chrimc62
chrimc62 approved these changes Jun 29, 2020
View reviewed changes
Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

:shipit:

@chrimc62
Copy link
Contributor

Fix two minor typos and good to go.

@Danieladu Danieladu merged commit d305b0b into master Jun 30, 2020
@Danieladu Danieladu deleted the hond/perf branch June 30, 2020 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrimc62 chrimc62 chrimc62 approved these changes

@boydc2014 boydc2014 Awaiting requested review from boydc2014

@luhan2017 luhan2017 Awaiting requested review from luhan2017

@stevengum stevengum Awaiting requested review from stevengum

@Stevenic Stevenic Awaiting requested review from Stevenic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[PORT] Further improve LG's performance
3 participants
@Danieladu @coveralls @chrimc62