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

refactor(utils): replace with lodash #1694

Merged
merged 23 commits into from
Nov 4, 2021
Merged

refactor(utils): replace with lodash #1694

merged 23 commits into from
Nov 4, 2021

Conversation

bd82
Copy link
Member

@bd82 bd82 commented Nov 1, 2021

WIP

@NaridaL
Copy link
Collaborator

NaridaL commented Nov 1, 2021

I think import from "lodash" with tree shaking also works (might need lodash-es).

If you want to go this way you should add https://eslint.org/docs/rules/no-restricted-imports with "lodash".

@bd82
Copy link
Member Author

bd82 commented Nov 1, 2021

I think import from "lodash" with tree shaking also works, (might need lodash-es).

Without lodash-es it bundled the whole lodash package.
I will try lodash-es

@bd82
Copy link
Member Author

bd82 commented Nov 1, 2021

Adding import {isEmpty} from "lodash-es" increased the bundle size from 166kb to 280kb.
I suspect it may be because we are first transpiling to ES5.1 using TSC and only than minifying.

@NaridaL
Copy link
Collaborator

NaridaL commented Nov 1, 2021

The problem is more likely the tsconfig module option. That should be set to ES and then webpack/esbuild should handle the conversion to cjs for the bundle.

@bd82
Copy link
Member Author

bd82 commented Nov 1, 2021

The problem is more likely the tsconfig module option. That should be set to ES and then webpack/esbuild should handle the conversion to cjs for the bundle.

Good point. 👍
Although I will probably finish the utils replacement without changes to tsconfig as it consumes too much time anyhow
and leave other changes as a separate possible future upgarde.

@bd82
Copy link
Member Author

bd82 commented Nov 1, 2021

Looks like this will cause a 15-20% increase in the bundles size, which I consider reasonable considering
the improvements in maintainability of the project.

Some of this increase will be nullified with the upcoming deprecation of features such as syntactic content assist.

Might be worth looking into more ways to decrease the bundle size, although that is not the highest priority...

@bd82
Copy link
Member Author

bd82 commented Nov 3, 2021

Need to:

  • check if there is an eslint rule to ensure lodash functions are imported with import foo from "lodash/foo" style.
    • Edit: yes there is: eslint-plugin-lodash
  • check if the change in the lookahead data structures from sparse array to object dictionary causes any meaningful performance regression (_.has assumes the array is packed and uses the length to return true/false)

Edit:

Object dictionary seems to have a slight performance advantage.
I guess it depends how "holey" the array becomes and under the hood details of V8

image

@@ -0,0 +1,14 @@
export function PRINT_ERROR(msg: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just move these into the chevrotain package?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • There may be future utils that will be common/shared, so that gave me an incentive to keep the utils bundle.
  • I believe the remaining three utils are all used in both the Parser and the Lexer, and I would like at some point in the future to extract the lexer to a separate package.

@@ -28,7 +28,7 @@ module.exports = {
parserOptions: {
// The `ecmaVersion` should align to the supported features of our target runtimes (browsers / nodejs / others)
// Consult with: https://kangax.github.io/compat-table/es2016plus/
ecmaVersion: 2017
ecmaVersion: 2018
Copy link
Member Author

@bd82 bd82 Nov 4, 2021

Choose a reason for hiding this comment

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

this is a JS only option, for scripts / configuration which enables using ... syntax

@bd82
Copy link
Member Author

bd82 commented Nov 4, 2021

There is no proper benchmark for this, but it seems introducing lodash may have decreased the parser/lexer init time by ~10%
Possibly due to deferring certain utils to the native implementations where possible.

@@ -203,7 +201,7 @@ export function buildAlternativesLookAheadFunc(
})
return result
},
[]
{} as Record<number, number>
Copy link
Member Author

Choose a reason for hiding this comment

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

has in lodash on array assumes the array is non-holey and seems to use the length property.
Changing to an object dictionary seems to have very slightly improved performance as well.

@bd82 bd82 merged commit 2dce611 into master Nov 4, 2021
@bd82 bd82 deleted the utils_and_lodash branch November 4, 2021 15:13
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