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

Name intermediate Leaf types #99

Merged
merged 5 commits into from
May 5, 2020
Merged

Name intermediate Leaf types #99

merged 5 commits into from
May 5, 2020

Conversation

Eyas
Copy link
Collaborator

@Eyas Eyas commented May 5, 2020

Rather than inlining a "leaf" type as an intersection in a big union,
name it. This should hopefully help with #34.

This is based on advice from @amcasey regarding compiler performance:

I've definitely seen some impressive perf improvements from naming
intermediate types (because it can short-circuit structural type
comparison, which is very expensive).

This fits neatly with my initial description of the Schema.org type
system here:
https://blog.eyas.sh/2019/05/modeling-schema-org-schema-with-typescript-the-power-and-limitations-of-the-typescript-type-system/

There's still some room for improvements:

  • Some intermediate types are simply aliases, can we remove these?
  • 'DataType's don't necessarily fit neatly here.

Also in this PR,

  • Remove unnecessary parens from certain type unions.

Eyas added 5 commits May 4, 2020 22:19
Rather than inlining a "leaf" type as an intersection in a big union,
name it. This should hopefully help with google#34.

This is based on advice from @amcasey regarding compiler performance:

> I've definitely seen some impressive perf improvements from naming
> intermediate types (because it can short-circuit structural type
> comparison, which is very expensive).

This fits neatly with my initial description of the Schema.org type
system here:
https://blog.eyas.sh/2019/05/modeling-schema-org-schema-with-typescript-the-power-and-limitations-of-the-typescript-type-system/

There's still some room for improvements:

- Some intermediate types are simply aliases, can we remove these?
- 'DataType's don't necessarily fit neatly here.
@Eyas Eyas changed the title Name intermediate Leaf types. … Name intermediate Leaf types May 5, 2020
@Eyas Eyas merged commit 95ed0a3 into google:master May 5, 2020
@amcasey
Copy link

amcasey commented May 5, 2020

@Eyas Cool! What sort of perf gain did you see?

For the benefit of future readers, I should clarify that when naming intermediate types helps (which is rare), it can help a lot.

@Eyas
Copy link
Collaborator Author

Eyas commented May 5, 2020

It makes a bit of a (positive) difference in 3.7 and 3.8, but might have regressed in 3.9.1. 95% confidence interval is pretty wide though.

TS 3.7:

  • 0.5.2: Mean memory of 5 runs: 95179.2K (95 CI: ±3,443)
  • 0.5.3: Mean memory of 5 runs: 92897.2K (95 CI: ±2,697)

TS 3.8:

  • 0.5.2: Mean memory of 5 runs: 100053K (95 CI: ±1,004)
  • 0.5.3: Mean memory of 5 runs: 94875K (95 CI: ±2,677)

TS 3.9.1-rc:

  • 0.5.2: Mean memory of 5 runs: 96871.6K (95 CI: ±231)
  • 0.5.3: Mean memory of 5 runs: 99921K (95 CI: ±2,271)

@Eyas
Copy link
Collaborator Author

Eyas commented May 5, 2020

This is using --extendedDiagnostics and looking at "Memory used". The repro case is simple:

import { WithContext, Article } from "schema-dts";

export const article: WithContext<Article> = {
    "@context": "https://schema.org",
    "@type": "Article",
    name: [
        "Woo", "Hoo"
    ]
};

with either schema-dts version installed. Node 13.

@amcasey
Copy link

amcasey commented May 6, 2020

Thanks! One day, I'll figure out why node memory usage is so variable. 😉

@Eyas Eyas deleted the leaf branch May 18, 2020 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants