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

Improve performance by reducing array slices and RegExp recreation #2128

Merged
merged 2 commits into from Jan 28, 2024

Conversation

hsource
Copy link
Contributor

@hsource hsource commented Jan 28, 2024

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

Motivation

Our app recently switched to using i18next. It works great, but the app felt slower. We have hundreds of strings on-screen at a time, so i18next code is run a lot.

After profiling, some i18next functions were the top contributors to time taken within our 20s stacktrace:

  • off: used by react-i18next (and base library i18next) for detaching event handlers on unmount. It looks like it calls a array.filter according to source code. This may be iterating through a very large array
  • getLastOfPath: 158ms - code does a lot of recursive retrieval and also creates a RegExp, which may take longer
  • looksLikeObjectPath: 293ms - code constructs RegExp
  • getResource: 221ms - code runs concat and deepFind, which does a lot of O(n) array operations

Looking at the causes, the 2 main causes are:

  1. Linear time array operations like .slice, .concat
  2. RegExp construction, which may be slow in the React Native Android runtime in particular

Fix

  • Change EventEmitter to use a Map instead of an array, changing off from an O(n) to O(1)
  • Cache regular expression construction for looksLikeObjectPath
    • Store expressions in a Map, there are only 32 possible RegExp that can be constructed so memory is trivial
  • Improve performance of resetRegExp
    • Rather than creating a new RegExp every time, just set lastIndex = 0 to reset a global RegExp state
  • Improve deepFind performance
    • Change recursive call to iterative
    • Perform less array copies and slice , use indexing and string appending instead of join
    • Changed parts from O(n^2) to O(n)
  • Improve getLastOfPath performance
    • Don't create a new RegExp every time, set lastIndex = 0
    • Use regular indexing O(1) rather than array.shift O(n)
  • Improve getResource performance
    • Don't copy arrays multiple times, just create array once and push items into it

Testing

  1. Added new automated tests for deepFind and eventEmitter.test.ts
  2. Used a branch of the code in our app and verified the performance improvements. The time spent in translation code decreased from about 700ms to 50-100ms in total

@adrai
Copy link
Member

adrai commented Jan 28, 2024

very interesting... but currently afk... not able to review this... but the tests seems to fail... can you check this?

@hsource
Copy link
Contributor Author

hsource commented Jan 28, 2024

I just pushed a fix to the linter error for using for..of: I went back to using .forEach which does the same thing. Take a look when you're free!

@coveralls
Copy link

Coverage Status

coverage: 96.214% (+0.06%) from 96.154%
when pulling 7a4426e on wanderlog:harry-optimize
into cf6b91f on i18next:master.

@adrai adrai merged commit 6127226 into i18next:master Jan 28, 2024
6 checks passed
@adrai
Copy link
Member

adrai commented Jan 28, 2024

Looks good. Thank you for your valuable contribution.
v23.8.0 has just been released.

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.

None yet

3 participants