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

[RFC] Speed up react-docgen by 25-40% #237

Closed
wants to merge 1 commit into from

Conversation

sophiebits
Copy link
Member

I did some profiling and noticed that recast has a significant amount of overhead due to the fact that it copies every tree it parses and has complicated printing logic. I wasn't sure if react-docgen uses any mutative recast features so I tried to remove it.

The good news: this is about 25-40% faster in my testing (40% faster when run on 20 files, 25% faster when run on 700 components from FB internal repos -- probably the JIT mitigates some of the slowness of recast).

The bad news: this doesn't actaully work, because (surprise, surprise) we actually do use some of the modification features of recast. When these are used, we don't know how to reprint.

$ ag --ignore __tests__ builders\\.
src/utils/normalizeClassDefinition.js
79:            var classProperty = builders.classProperty(

src/utils/resolveObjectKeysToArray.js
98:        .map(value => builders.literal(value))
100:      return new NodePath(builders.arrayExpression(nodes));

src/utils/resolveToValue.js
33:        builders.memberExpression(

Maybe we could reimplement a small subset of the printing logic? Or maybe that's too impractical. FWIW all but one of the 700 files I tested produced the exact same output including whitespace in the raw JS source fields.

I did some profiling and noticed that recast has a significant amount of overhead due to the fact that it copies every tree it parses and has complicated printing logic. I wasn't sure if react-docgen uses any mutative recast features so I tried to remove it.

The good news: this is about 25-40% faster in my testing (40% faster when run on 20 files, 25% faster when run on 700 components from FB internal repos -- probably the JIT mitigates some of the slowness of recast).

The bad news: this doesn't actaully work, because (surprise, surprise) we actually do use some of the modification features of recast. When these are used, we don't know how to reprint.

```
$ ag --ignore __tests__ builders\\.
src/utils/normalizeClassDefinition.js
79:            var classProperty = builders.classProperty(

src/utils/resolveObjectKeysToArray.js
98:        .map(value => builders.literal(value))
100:      return new NodePath(builders.arrayExpression(nodes));

src/utils/resolveToValue.js
33:        builders.memberExpression(
```

Maybe we could reimplement a small subset of the printing logic? Or maybe that's too impractical. FWIW all but one of the 700 files I tested produced the exact same output including whitespace in the `raw` JS source fields.
@sophiebits
Copy link
Member Author

(FB-only context for why I ended up looking at this: https://fburl.com/zbfp3cl0.)

@danez danez closed this Jun 26, 2018
@danez danez changed the base branch from v3-dev to master June 26, 2018 14:10
@danez danez reopened this Jun 26, 2018
@danez danez mentioned this pull request Feb 8, 2019
@danez
Copy link
Collaborator

danez commented Apr 26, 2019

Closing this now as I created #349 based on your idea.

The reprinting was actually only failing in the case of resolveObjectKeysToArray and resolveObjectKeysToArray where we create literals with builders, all the other cases are either never reprinted or contain nodes from the original AST.

Thank you very much.

Out of curiosity what was the FB internal reason to look into this? Is it too slow?

@danez danez closed this Apr 26, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants