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

Fix #1266, make SourceCode only for no-deprecated rule #1271

Closed

Conversation

sergei-startsev
Copy link
Contributor

Parsing source code allocates a lot of memory since SourceCode instances are built recursively for each node in the dependency graph:
https://github.com/benmosher/eslint-plugin-import/blob/1ec80fa35fa1819e2d35a70e68fb6a149fb57c5e/src/ExportMap.js#L349

SourceCode instance is only used to capture docs from leading comments that are actually used only for no-deprecated rule:
https://github.com/benmosher/eslint-plugin-import/blob/1ec80fa35fa1819e2d35a70e68fb6a149fb57c5e/src/ExportMap.js#L417-L418

https://github.com/benmosher/eslint-plugin-import/blob/1ec80fa35fa1819e2d35a70e68fb6a149fb57c5e/src/ExportMap.js#L453-L454

https://github.com/benmosher/eslint-plugin-import/blob/1ec80fa35fa1819e2d35a70e68fb6a149fb57c5e/src/ExportMap.js#L456-L459

However, SourceCode instances are built even if no-deprecated rule is turned off (btw the rule is in Stage 0 and is still work in progress). The PR prevents negative impact on other rules.

Here's memory consumption before/after PR for pretty large code base, no-deprecated rule is disabled for both cases:

image

The 1st process failed with heap out of memory error.

src/ExportMap.js Outdated
@@ -346,7 +348,7 @@ ExportMap.parse = function (path, content, context) {
docStyleParsers[style] = availableDocStyleParsers[style]
})

const source = makeSourceCode(content, ast)
const source = context.id === 'no-deprecated' ? makeSourceCode(content, ast) : null
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make this metadata the rule can supply rather than hardcoding a single rule name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'll take a look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb It seems metadata doesn't provide an appropriate field: https://eslint.org/docs/developer-guide/working-with-rules#rule-basics

Copy link
Member

Choose a reason for hiding this comment

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

perhaps an argument passed to ExportMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing an argument looks better, it seems rule id isn't passed in ESlint 2-3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but more changes are required... 😓

Copy link
Member

Choose a reason for hiding this comment

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

maybe instead of skipping the comments, we could make them lazy?

ie, instead of using properties, use functions that lazily create the source code object as needed (but memoize it across calls)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid it's too late, we don't have required data (content, ast) when we need it, otherwise we have to store them as well

@sergei-startsev
Copy link
Contributor Author

@ljharb any ideas regarding appveyor build failure?:

Error: Command failed: git cat-file -p f71edacae52498377210e97c5540da50f80f3b2b
fatal: Not a valid object name f71edacae52498377210e97c5540da50f80f3b2b

I think I've already seen this in other PRs...

@coveralls
Copy link

coveralls commented Jan 27, 2019

Coverage Status

Coverage increased (+0.004%) to 97.311% when pulling fc26273 on sergei-startsev:no-deprecated-1266 into 1ec80fa on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 97.31% when pulling d1715d2 on sergei-startsev:no-deprecated-1266 into 1ec80fa on benmosher:master.

@benmosher
Copy link
Member

Closing in deference to #1275

@benmosher benmosher closed this Jan 29, 2019
@benmosher
Copy link
Member

Thanks for taking a stab at this, though! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants