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

Better perf with differential parsing #22

Open
siddharthkp opened this issue Aug 18, 2016 · 13 comments
Open

Better perf with differential parsing #22

siddharthkp opened this issue Aug 18, 2016 · 13 comments

Comments

@siddharthkp
Copy link
Owner

siddharthkp commented Aug 18, 2016

Reduce the number of files parsed in each cycle.

Read all the files only delete event.

Chain events?

@ravinsinghd
Copy link

Shall work on this?
my idea was on change event call the main function with file path, so in getUsedModules function we can use the path and get the module from that particular file instead of all files.

@siddharthkp
Copy link
Owner Author

Sure!

Just one thing to remember, when a file is deleted, you will have to go through all the files as that file is no longer available.

@ravinsinghd
Copy link

OK thanks will update once done

@siddharthkp
Copy link
Owner Author

If you get stuck anywhere because of my code, don't hesitate to ask

@ravinsinghd
Copy link

ravinsinghd commented Feb 14, 2017

updated the code like below
if (filePath) usedModules = helpers.getUsedModules(filePath);
else usedModules = helpers.getUsedModules();

but the problem now, because in change event we are getting module from one page, all other page modules consider as unused modules by following line of code

let unusedModules = helpers.diff(installedModules, usedModules);

any suggestion how can handle this?

@siddharthkp
Copy link
Owner Author

helpers.diff will compare between the 2 arrays you pass to it.

Implementing path specific logic for helpers.getInstalledModules and helpers.getUsedModules both will solve this problem.

@ravinsinghd
Copy link

check is the below steps are OK.

  1. Start

  2. Get used modules by file parsing(path specific)

  3. Save used module in local variable

  4. Get installed modules by parsing package.json

  5. Find diff do action (mostly install module)

  6. start watcher

  7. on change event get modules from file which changed

  8. compare the module with path specific modules list in local variable

  9. after diff do action (install || uninstall)

  10. update the local variable with new modules (just update the object property)

  11. on delete get the file path

  12. get modules for this files from local variable

  13. check if the modules used in any other files using local variable

  14. if used no action

  15. else uninstall modules

  16. update the local variable with new modules (remove the obj property which belong to file)

@siddharthkp
Copy link
Owner Author

Nothing wrong with your approach, but personally I'm not a fan of storing these details in a local variable unless there is no other approach. More logic, more bugs? Would prefer a pure function instead.

In the current implementation, I just rerun the same function every time. But, parsing all the files is an expensive action.

Relevant use cases in the current implementation,

  1. When a module is added to a file, even though only one file has changed, it parses all the files. This can be optimised heavily.
  2. When a module is removed from a file, we have to parse all the files to see if it was used somewhere else too.
  3. When a file is removed, we don't have access to it anymore, and do a clean run through all the files (same as the first time run)

I think that the first point is the most common use case and will benefit the most.

That being said, I don't want to discourage your implementation, if you can make the local store clean + testable, the performance benefits will be totally worth it.

@ravinsinghd
Copy link

ok will do my best :)

@ravinsinghd
Copy link

hi,
i am working on this fork. can you validate the code am i going right.

@siddharthkp
Copy link
Owner Author

Seems about right

@ravinsinghd
Copy link

Thanks for your time. I ll continue same way

@ravinsinghd
Copy link

hi
on following line i am concat the new modules with existing local variable. but the problem chokidar watcher still hold the old value for modules list. any idea how we can update that one?

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

No branches or pull requests

2 participants