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 top level logical assignment leak #40536

Merged
merged 3 commits into from
Mar 14, 2022
Merged

Fix top level logical assignment leak #40536

merged 3 commits into from
Mar 14, 2022
+270 −1

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Sep 14, 2020 •
edited
Loading

Fixes #40494
Fixes #41153

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Sep 14, 2020
@Kingwl
Copy link
Contributor Author

Kingwl commented Sep 15, 2020

Ci failed but not my fault.

@Kingwl
Copy link
Contributor Author

Kingwl commented Sep 18, 2020

@typescript-bot pack this.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 18, 2020 •
edited
Loading

Heya @Kingwl, I've started to run the tarball bundle task on this PR at 9d246a2. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 18, 2020 •
edited
Loading

Hey @Kingwl, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/85734/artifacts?artifactName=tgz&fileId=083AF70D84F97F52F27365D2997BC6812F0E94F83D3959CECD2F294BED3E351102&fileName=/typescript-4.1.0-insiders.20200918.tgz"
    }
}

and then running npm install.

There is also a playground for this build.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Oct 20, 2020
@Kingwl
Copy link
Contributor Author

Kingwl commented Oct 30, 2020

Emmm.... It's better to merge before 4.1 IMO
CC:/ @sandersn

@Kingwl
Copy link
Contributor Author

Kingwl commented Dec 31, 2020

UUUP

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 10, 2021

Emmmm.. Friendly Ping again

@Kingwl
Copy link
Contributor Author

Kingwl commented Mar 25, 2021 •
edited
Loading

Sorry. Are there anything else to do for this fix?....

sandersn
sandersn requested changes Mar 31, 2021
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I don't understand why this is the correct fix.

src/compiler/binder.ts
@@ -1072,7 +1072,6 @@ namespace ts {
node = node.parent;
}
return !isStatementCondition(node) &&
!isLogicalAssignmentExpression(node.parent) &&
Copy link
Member

@sandersn sandersn Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This function is called when node was a LogicalAssignmentOperator. But from the tests, I guess that the failing case was when node was a LogicalOperator and node.parent was a LogicalAssignmentOperator. I don't understand why the control flow doesn't get created when the parent LogicalAssignmentOperator is node.

Also, are there now more control flow nodes created for other nodes besides LogicalAssignmentOperators? I'll run the perf tests.

Copy link
Member

Choose a reason for hiding this comment

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

Much later: I was wrong. It is called when node is a QuestionQuestionToken and node.parent is a LogicalAssignmentExpression. From the new test, when node is x in d ??= x ?? "string", and node.parent is ??=.

I guess the deleted line was added because ??= is a logical expression...but it's also an assignment expression.

@sandersn
Copy link
Member

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 31, 2021 •
edited
Loading

Heya @sandersn, I've started to run the perf test suite on this PR at 5582f6a. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@sandersn
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..40536

Metric master 40536 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 344,957k (± 0.02%) 344,897k (± 0.03%) -60k (- 0.02%) 344,665k 345,128k
Parse Time 1.93s (± 0.62%) 1.94s (± 0.53%) +0.00s (+ 0.21%) 1.91s 1.96s
Bind Time 0.84s (± 1.07%) 0.83s (± 0.60%) -0.00s (- 0.12%) 0.83s 0.85s
Check Time 5.10s (± 0.51%) 5.11s (± 0.57%) +0.01s (+ 0.14%) 5.07s 5.19s
Emit Time 5.94s (± 0.88%) 5.92s (± 0.71%) -0.02s (- 0.39%) 5.87s 6.07s
Total Time 13.81s (± 0.61%) 13.80s (± 0.52%) -0.01s (- 0.09%) 13.72s 14.07s
Compiler-Unions - node (v10.16.3, x64)
Memory used 203,277k (± 0.02%) 203,287k (± 0.05%) +10k (+ 0.00%) 203,137k 203,566k
Parse Time 0.79s (± 1.05%) 0.78s (± 0.93%) -0.00s (- 0.51%) 0.77s 0.80s
Bind Time 0.52s (± 1.13%) 0.52s (± 1.40%) -0.01s (- 1.15%) 0.50s 0.53s
Check Time 7.48s (± 0.68%) 7.47s (± 0.61%) -0.01s (- 0.11%) 7.38s 7.55s
Emit Time 2.59s (± 1.06%) 2.58s (± 0.75%) -0.01s (- 0.31%) 2.54s 2.62s
Total Time 11.38s (± 0.59%) 11.35s (± 0.43%) -0.02s (- 0.21%) 11.26s 11.46s
Monaco - node (v10.16.3, x64)
Memory used 342,644k (± 0.01%) 342,674k (± 0.02%) +30k (+ 0.01%) 342,573k 342,827k
Parse Time 1.55s (± 0.45%) 1.56s (± 0.85%) +0.01s (+ 0.84%) 1.54s 1.61s
Bind Time 0.74s (± 0.79%) 0.74s (± 0.63%) +0.00s (+ 0.41%) 0.73s 0.75s
Check Time 5.24s (± 0.49%) 5.26s (± 0.45%) +0.01s (+ 0.25%) 5.23s 5.33s
Emit Time 3.11s (± 0.83%) 3.12s (± 0.55%) +0.01s (+ 0.29%) 3.08s 3.15s
Total Time 10.64s (± 0.44%) 10.68s (± 0.30%) +0.04s (+ 0.38%) 10.61s 10.77s
TFS - node (v10.16.3, x64)
Memory used 304,437k (± 0.09%) 304,258k (± 0.01%) -180k (- 0.06%) 304,159k 304,345k
Parse Time 1.21s (± 0.78%) 1.21s (± 0.55%) -0.00s (- 0.00%) 1.20s 1.23s
Bind Time 0.70s (± 0.95%) 0.70s (± 0.68%) 0.00s ( 0.00%) 0.69s 0.71s
Check Time 4.75s (± 0.48%) 4.74s (± 0.62%) -0.01s (- 0.21%) 4.68s 4.84s
Emit Time 3.26s (± 1.67%) 3.24s (± 1.05%) -0.02s (- 0.52%) 3.15s 3.32s
Total Time 9.92s (± 0.68%) 9.90s (± 0.59%) -0.03s (- 0.25%) 9.78s 10.07s
material-ui - node (v10.16.3, x64)
Memory used 465,390k (± 0.01%) 465,416k (± 0.01%) +26k (+ 0.01%) 465,269k 465,530k
Parse Time 2.01s (± 0.61%) 2.00s (± 0.38%) -0.01s (- 0.60%) 1.98s 2.01s
Bind Time 0.66s (± 1.34%) 0.66s (± 0.93%) +0.00s (+ 0.61%) 0.64s 0.67s
Check Time 14.30s (± 0.85%) 14.24s (± 0.29%) -0.06s (- 0.43%) 14.16s 14.35s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.97s (± 0.75%) 16.90s (± 0.26%) -0.07s (- 0.40%) 16.80s 17.01s
Angular - node (v12.1.0, x64)
Memory used 322,657k (± 0.02%) 322,695k (± 0.03%) +38k (+ 0.01%) 322,473k 322,863k
Parse Time 1.92s (± 0.60%) 1.91s (± 0.40%) -0.01s (- 0.47%) 1.90s 1.93s
Bind Time 0.81s (± 1.09%) 0.82s (± 0.93%) +0.00s (+ 0.25%) 0.80s 0.83s
Check Time 5.00s (± 0.46%) 5.00s (± 0.43%) +0.00s (+ 0.02%) 4.96s 5.07s
Emit Time 5.98s (± 0.78%) 5.96s (± 0.35%) -0.02s (- 0.30%) 5.92s 6.02s
Total Time 13.71s (± 0.40%) 13.69s (± 0.14%) -0.02s (- 0.17%) 13.64s 13.73s
Compiler-Unions - node (v12.1.0, x64)
Memory used 190,470k (± 0.13%) 190,386k (± 0.19%) -85k (- 0.04%) 189,273k 190,886k
Parse Time 0.77s (± 0.91%) 0.76s (± 0.48%) -0.01s (- 0.65%) 0.76s 0.77s
Bind Time 0.53s (± 0.98%) 0.52s (± 0.57%) -0.01s (- 1.32%) 0.52s 0.53s
Check Time 6.98s (± 0.74%) 6.97s (± 0.51%) -0.02s (- 0.21%) 6.90s 7.06s
Emit Time 2.55s (± 1.31%) 2.54s (± 0.94%) -0.01s (- 0.47%) 2.50s 2.61s
Total Time 10.83s (± 0.73%) 10.79s (± 0.31%) -0.04s (- 0.37%) 10.72s 10.85s
Monaco - node (v12.1.0, x64)
Memory used 325,089k (± 0.02%) 325,051k (± 0.02%) -38k (- 0.01%) 324,948k 325,187k
Parse Time 1.54s (± 0.75%) 1.53s (± 0.73%) -0.00s (- 0.13%) 1.50s 1.55s
Bind Time 0.72s (± 0.55%) 0.72s (± 0.47%) -0.00s (- 0.55%) 0.71s 0.72s
Check Time 5.09s (± 0.54%) 5.09s (± 0.49%) -0.00s (- 0.08%) 5.04s 5.13s
Emit Time 3.10s (± 0.54%) 3.09s (± 0.53%) -0.01s (- 0.23%) 3.05s 3.13s
Total Time 10.45s (± 0.43%) 10.43s (± 0.40%) -0.02s (- 0.17%) 10.34s 10.53s
TFS - node (v12.1.0, x64)
Memory used 288,818k (± 0.02%) 288,790k (± 0.02%) -29k (- 0.01%) 288,690k 288,907k
Parse Time 1.22s (± 0.68%) 1.20s (± 0.51%) -0.01s (- 1.15%) 1.19s 1.22s
Bind Time 0.70s (± 0.85%) 0.69s (± 1.25%) -0.00s (- 0.57%) 0.68s 0.72s
Check Time 4.66s (± 0.72%) 4.65s (± 0.47%) -0.00s (- 0.02%) 4.61s 4.73s
Emit Time 3.16s (± 0.84%) 3.13s (± 0.78%) -0.02s (- 0.76%) 3.06s 3.17s
Total Time 9.72s (± 0.62%) 9.68s (± 0.41%) -0.04s (- 0.45%) 9.58s 9.77s
material-ui - node (v12.1.0, x64)
Memory used 443,550k (± 0.02%) 443,586k (± 0.02%) +36k (+ 0.01%) 443,375k 443,701k
Parse Time 2.03s (± 0.54%) 2.02s (± 0.32%) -0.01s (- 0.59%) 2.01s 2.04s
Bind Time 0.64s (± 0.47%) 0.64s (± 0.93%) -0.00s (- 0.31%) 0.62s 0.65s
Check Time 12.92s (± 0.73%) 12.83s (± 0.35%) -0.09s (- 0.70%) 12.74s 12.95s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.59s (± 0.60%) 15.49s (± 0.27%) -0.10s (- 0.67%) 15.39s 15.59s
Angular - node (v14.15.1, x64)
Memory used 321,368k (± 0.01%) 321,386k (± 0.01%) +18k (+ 0.01%) 321,338k 321,431k
Parse Time 1.93s (± 0.42%) 1.92s (± 0.49%) -0.01s (- 0.52%) 1.91s 1.95s
Bind Time 0.86s (± 0.57%) 0.86s (± 0.65%) -0.01s (- 0.69%) 0.85s 0.87s
Check Time 5.02s (± 0.58%) 5.01s (± 0.60%) -0.01s (- 0.18%) 4.94s 5.07s
Emit Time 6.33s (± 0.94%) 6.32s (± 0.38%) -0.02s (- 0.25%) 6.27s 6.36s
Total Time 14.15s (± 0.54%) 14.11s (± 0.30%) -0.04s (- 0.28%) 14.03s 14.21s
Compiler-Unions - node (v14.15.1, x64)
Memory used 189,397k (± 0.02%) 189,422k (± 0.01%) +24k (+ 0.01%) 189,357k 189,473k
Parse Time 0.80s (± 0.62%) 0.80s (± 0.83%) -0.00s (- 0.62%) 0.79s 0.82s
Bind Time 0.56s (± 1.00%) 0.56s (± 0.85%) -0.00s (- 0.18%) 0.55s 0.57s
Check Time 7.12s (± 0.73%) 7.04s (± 0.50%) -0.08s (- 1.15%) 6.96s 7.14s
Emit Time 2.56s (± 0.84%) 2.54s (± 0.68%) -0.01s (- 0.59%) 2.51s 2.58s
Total Time 11.04s (± 0.59%) 10.94s (± 0.42%) -0.10s (- 0.93%) 10.84s 11.08s
Monaco - node (v14.15.1, x64)
Memory used 324,052k (± 0.01%) 324,041k (± 0.01%) -11k (- 0.00%) 323,992k 324,096k
Parse Time 1.58s (± 0.33%) 1.56s (± 0.45%) -0.02s (- 1.14%) 1.55s 1.58s
Bind Time 0.75s (± 0.66%) 0.74s (± 0.64%) -0.00s (- 0.40%) 0.73s 0.75s
Check Time 5.03s (± 0.39%) 5.04s (± 0.46%) +0.02s (+ 0.36%) 5.01s 5.12s
Emit Time 3.15s (± 0.52%) 3.17s (± 0.62%) +0.01s (+ 0.44%) 3.14s 3.23s
Total Time 10.51s (± 0.21%) 10.52s (± 0.37%) +0.01s (+ 0.13%) 10.48s 10.65s
TFS - node (v14.15.1, x64)
Memory used 287,682k (± 0.01%) 287,682k (± 0.01%) -0k (- 0.00%) 287,641k 287,741k
Parse Time 1.27s (± 1.08%) 1.27s (± 1.67%) -0.01s (- 0.47%) 1.24s 1.34s
Bind Time 0.72s (± 1.01%) 0.71s (± 0.67%) -0.01s (- 0.70%) 0.70s 0.72s
Check Time 4.67s (± 0.44%) 4.66s (± 0.49%) -0.01s (- 0.13%) 4.63s 4.73s
Emit Time 3.28s (± 0.70%) 3.25s (± 0.57%) -0.03s (- 0.85%) 3.21s 3.31s
Total Time 9.94s (± 0.36%) 9.89s (± 0.36%) -0.04s (- 0.43%) 9.83s 9.97s
material-ui - node (v14.15.1, x64)
Memory used 441,829k (± 0.00%) 441,834k (± 0.00%) +5k (+ 0.00%) 441,791k 441,875k
Parse Time 2.10s (± 0.43%) 2.10s (± 0.51%) -0.01s (- 0.29%) 2.06s 2.11s
Bind Time 0.70s (± 0.71%) 0.69s (± 0.50%) -0.01s (- 1.29%) 0.68s 0.69s
Check Time 13.07s (± 0.89%) 12.97s (± 0.72%) -0.10s (- 0.78%) 12.83s 13.28s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.87s (± 0.74%) 15.75s (± 0.58%) -0.12s (- 0.74%) 15.61s 16.04s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-206-generic
Architecturex64
Available Memory16 GB
Available Memory10 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
Benchmark Name Iterations
Current 40536 10
Baseline master 10

Developer Information:

Download Benchmark

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 1, 2021 •
edited
Loading

Sorry, I'm a little unsure.

If I'm correct, Logical assignment operator will use the existed control flow for below code, but logical operator create new one to limit the control flow changes inside the expression.

So The function isTopLevelLogicalExpression might not an approximate name. I think check should called something like shouldCreateNewControlFlowForLogicalExpression.

@sandersn
Copy link
Member

sandersn commented Apr 2, 2021

That sounds like a more accurate name from my reading of the code too. But I still don't understand why the deleted code checks that node.parent and not node is a logical assignment operator. Can you try to find out why that is the case?

@Kingwl
Copy link
Contributor Author

Kingwl commented Apr 5, 2021

We only check a node is a "top level logical expression" if the node is a logical expression.

If a node is "top level", the parent node should not be an logical expression. Otherwise the parent node is the top level.

Seems reasonable doesn't it?.😂

sandersn
sandersn approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Based on my understanding of control flow (which isn't great here), and the fact that this doesn't hurt performance in projects that don't use ??=, I think this is good to go.

src/compiler/binder.ts
@@ -1072,7 +1072,6 @@ namespace ts {
node = node.parent;
}
return !isStatementCondition(node) &&
!isLogicalAssignmentExpression(node.parent) &&
Copy link
Member

Choose a reason for hiding this comment

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

Much later: I was wrong. It is called when node is a QuestionQuestionToken and node.parent is a LogicalAssignmentExpression. From the new test, when node is x in d ??= x ?? "string", and node.parent is ??=.

I guess the deleted line was added because ??= is a logical expression...but it's also an assignment expression.

@sandersn sandersn merged commit 5f017df into microsoft:main Mar 14, 2022
@Kingwl Kingwl deleted the fix_top_level_logical_assignment_leak branch March 15, 2022 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandersn sandersn

@ahejlsberg ahejlsberg

Assignees

@ahejlsberg ahejlsberg

Labels
For Backlog Bug PRs that fix a backlog bug For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
Archived in project
Milestone
No milestone
4 participants
@Kingwl @typescript-bot @sandersn @ahejlsberg