Skip to content

Strip tags in placeholders #56458

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
Closed
wants to merge 8 commits into from
Closed

Strip tags in placeholders #56458

wants to merge 8 commits into from

Conversation

ahejlsberg
Copy link
Member

This PR builds on #56434 to strip object type tags in template literal placeholders. We can merge this in case we decide to go that way.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 19, 2023
@ahejlsberg
Copy link
Member Author

@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 19, 2023 •
edited
Loading

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 5741df4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 19, 2023 •
edited
Loading

Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at 5741df4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 19, 2023 •
edited
Loading

Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 5741df4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 19, 2023 •
edited
Loading

Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 5741df4. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v18.15.0, x64)
Memory used 295,195k (± 0.01%) 295,190k (± 0.01%) ~ 295,139k 295,226k p=0.873 n=6
Parse Time 2.64s (± 0.24%) 2.65s (± 0.28%) ~ 2.64s 2.66s p=0.081 n=6
Bind Time 0.82s (± 0.63%) 0.83s (± 1.08%) ~ 0.82s 0.84s p=0.190 n=6
Check Time 8.05s (± 0.24%) 8.05s (± 0.16%) ~ 8.03s 8.06s p=1.000 n=6
Emit Time 7.08s (± 0.32%) 7.08s (± 0.42%) ~ 7.03s 7.11s p=0.571 n=6
Total Time 18.59s (± 0.20%) 18.61s (± 0.24%) ~ 18.52s 18.64s p=0.192 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used 192,635k (± 1.54%) 191,684k (± 1.28%) ~ 190,651k 196,678k p=0.298 n=6
Parse Time 1.36s (± 0.93%) 1.36s (± 0.86%) ~ 1.35s 1.38s p=0.801 n=6
Bind Time 0.72s (± 0.00%) 0.72s (± 0.00%) ~ 0.72s 0.72s p=1.000 n=6
Check Time 9.18s (± 0.53%) 9.19s (± 0.39%) ~ 9.15s 9.23s p=1.000 n=6
Emit Time 2.64s (± 0.62%) 2.64s (± 0.20%) ~ 2.63s 2.64s p=0.560 n=6
Total Time 13.89s (± 0.35%) 13.90s (± 0.24%) ~ 13.87s 13.94s p=0.686 n=6
Monaco - node (v18.15.0, x64)
Memory used 347,363k (± 0.01%) 347,344k (± 0.00%) ~ 347,326k 347,367k p=0.109 n=6
Parse Time 2.46s (± 0.47%) 2.45s (± 0.40%) ~ 2.44s 2.46s p=0.209 n=6
Bind Time 0.92s (± 0.59%) 0.92s (± 0.56%) ~ 0.92s 0.93s p=0.640 n=6
Check Time 6.91s (± 0.32%) 6.92s (± 0.20%) ~ 6.90s 6.93s p=0.934 n=6
Emit Time 4.05s (± 0.13%) 4.04s (± 0.41%) ~ 4.01s 4.06s p=0.114 n=6
Total Time 14.35s (± 0.18%) 14.33s (± 0.17%) ~ 14.30s 14.37s p=0.373 n=6
TFS - node (v18.15.0, x64)
Memory used 302,616k (± 0.00%) 302,634k (± 0.01%) ~ 302,593k 302,708k p=0.470 n=6
Parse Time 2.01s (± 0.86%) 1.99s (± 1.14%) ~ 1.95s 2.01s p=0.124 n=6
Bind Time 1.00s (± 1.36%) 1.00s (± 1.17%) ~ 0.99s 1.02s p=0.550 n=6
Check Time 6.27s (± 0.30%) 6.28s (± 0.36%) ~ 6.25s 6.31s p=0.746 n=6
Emit Time 3.59s (± 0.65%) 3.57s (± 0.40%) ~ 3.55s 3.59s p=0.252 n=6
Total Time 12.87s (± 0.36%) 12.83s (± 0.19%) ~ 12.80s 12.86s p=0.170 n=6
material-ui - node (v18.15.0, x64)
Memory used 470,554k (± 0.00%) 470,565k (± 0.00%) ~ 470,538k 470,598k p=0.471 n=6
Parse Time 2.57s (± 0.40%) 2.57s (± 0.25%) ~ 2.56s 2.58s p=0.654 n=6
Bind Time 0.99s (± 0.41%) 1.00s (± 0.75%) +0.01s (+ 1.01%) 0.99s 1.01s p=0.024 n=6
Check Time 16.64s (± 0.41%) 16.67s (± 0.20%) ~ 16.61s 16.71s p=0.470 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 20.20s (± 0.34%) 20.23s (± 0.18%) ~ 20.17s 20.27s p=0.422 n=6
xstate - node (v18.15.0, x64)
Memory used 512,805k (± 0.01%) 512,797k (± 0.01%) ~ 512,766k 512,839k p=0.810 n=6
Parse Time 3.27s (± 0.23%) 3.27s (± 0.27%) ~ 3.26s 3.28s p=0.798 n=6
Bind Time 1.55s (± 0.33%) 1.54s (± 0.68%) ~ 1.53s 1.56s p=0.794 n=6
Check Time 2.85s (± 0.29%) 2.85s (± 0.48%) ~ 2.83s 2.87s p=0.195 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 4.99%) ~ 0.08s 0.09s p=0.405 n=6
Total Time 7.74s (± 0.13%) 7.75s (± 0.13%) ~ 7.73s 7.76s p=0.548 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Angular - node (v18.15.0, x64)
  • Compiler-Unions - node (v18.15.0, x64)
  • Monaco - node (v18.15.0, x64)
  • TFS - node (v18.15.0, x64)
  • material-ui - node (v18.15.0, x64)
  • xstate - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the user test suite comparing main and refs/pull/56458/merge:

There were infrastructure failures potentially unrelated to your change:

  • 2 instances of "Package install failed"

Otherwise...

Something interesting changed - please have a look.

Details

puppeteer

packages/browsers/test/src/tsconfig.json

@typescript-bot
Copy link
Collaborator

Hey @ahejlsberg, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

@typescript-bot
Copy link
Collaborator

@ahejlsberg Here are the results of running the top-repos suite comparing main and refs/pull/56458/merge:

Everything looks good!

@jakebailey
Copy link
Member

Given the leak problem is not new, I personally prefer #56434 unless there's some major performance wins happening. I still need to test it (though I think you said you did?)

@typescript-bot pack this

@epmatsw Is there any chance you could try this build compared to nightly on your codebase from #52345? This is not a revert of #48044 (in that it doesn't reduce everything to string), so I'm curious as to the perf difference. Maybe if you're bored you can also try #56434 (comment)

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 20, 2023 •
edited
Loading

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 5741df4. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Nov 20, 2023 •
edited
Loading

Hey @jakebailey, 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/158706/artifacts?artifactName=tgz&fileId=E6A8BCB0EF65FF921D64598A0CE099A22405C83217C5F0D64D8784C7C9564EA202&fileName=/typescript-5.4.0-insiders.20231120.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.4.0-pr-56458-11".;

@ahejlsberg ahejlsberg closed this Nov 21, 2023
@epmatsw
Copy link

epmatsw commented Nov 21, 2023

@jakebailey

Doesn't seem like either has much of an effect, but it does look like something in an upcoming release did!

Version Time (s)
5.2.2 67
5.3.2 67
5.4.0-dev.20231120 47
#56434 49
#56458 49

@jakebailey
Copy link
Member

Curious; would be interesting to bisect that using something like https://www.npmjs.com/package/every-ts, e.g.:

$ every-ts bisect start
$ every-ts bisect old 5.3.2
$ every-ts bisect new 5.4.0-dev.20231120
$ every-ts tsc ...
$ every-ts bisect <old|new>
...

@epmatsw
Copy link

epmatsw commented Nov 27, 2023

5.4.0-dev.20231107 seems to be the first "fast" version

@epmatsw
Copy link

epmatsw commented Nov 27, 2023

Which makes sense, that has #55926

@jakebailey
Copy link
Member

Oh, right. And that actually helped on your real-world case? That's good, at least, though a little suspect to be narrowing something to itself.

@jcalz jcalz mentioned this pull request Apr 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakebailey jakebailey Awaiting requested review from jakebailey

@DanielRosenwasser DanielRosenwasser Awaiting requested review from DanielRosenwasser

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ahejlsberg @typescript-bot @jakebailey @epmatsw