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

[Feat] Memoize getDefinition parser function #361

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jan 8, 2021

I've been working on schemas for Github webhook events with the goal of generating types from the schemas - we recently merged the initial schemas, and so I've gotten to the point of actually generating the types from the schema.json generated by running npm run build:schema on this pr.

The schema.json is quite large - just shy of 30000 lines, and currently it takes ~120 seconds to build the .d.ts file.
While I know this is to be expected when running large schemas, I did some basic poking around and found that the bulk of the bottleneck is in the getDefinitions parser method.

That method had a jsdoc block with the comment "TODO: Memoize" so on a whim I decided to see how fast it'd be if I did just that.

Turns out it'll be 96.4% times faster, taking the time from ~120 seconds down to ~3 seconds while still generating an identical schema.

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! A couple of minor nits.

src/parser.ts Outdated Show resolved Hide resolved
@G-Rath G-Rath requested a review from bcherny January 8, 2021 03:04
Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

Looks great!

@bcherny bcherny merged commit daf03a0 into bcherny:master Jan 8, 2021
@G-Rath G-Rath deleted the memoize-function branch January 8, 2021 03:06
@bcherny
Copy link
Owner

bcherny commented Jan 8, 2021

Published v10.1.2

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