Significantly improved addFile/changeFile/removeFile performance. #3726
Conversation
|
@googlebot - signed it! |
|
Thanks for the PR @jfstephe! The change looks very good 👍 As for the CLA, most likely it is failing because the commit and the signed CLA have different emails. |
jfstephe
force-pushed
the
fix-file-change-performance
branch
2 times, most recently
from
57740d6 to
a58023e
Compare
Change to the event handlers so that they don't return the, generally unecessary, list of files which can save around 100ms per-event (depending on machine spec) BREAKING CHANGE: Core public API change: Handlers no longer return the list of files, and a separate call is now needed to return that information.
jfstephe
force-pushed
the
fix-file-change-performance
branch
from
a58023e to
5b38dc0
Compare
|
@devoto13 - CLA corrected, along with the commit message format (I think!) :-). Question: Can you give me an idea of typical time-scales for this being released? If it's more than a couple of weeks I'll publish this to our private npm locally, if not I'll wait for a while. Thanks! |
|
@jfstephe Thank you for fixing the issues! I'm afraid it may take more than a couple of weeks before this gets released, so private package is probably the way to go. You should also be able to install your branch directly from GitHub using: |
|
Awesome, thanks! |
| @@ -172,18 +173,18 @@ class FileList { | |||
| const excluded = this._findExcluded(path) | |||
| if (excluded) { | |||
| log.debug(`Add file "${path}" ignored. Excluded by "${excluded}".`) | |||
| return this.files | |||
| return | |||
There was a problem hiding this comment.
@jginsburgn Yes, I think it should be better to land it in a major release.
| @@ -114,6 +114,7 @@ class FileList { | |||
| return lastCompletedRefresh | |||
| } | |||
|
|
|||
| // TODO - Change this so that it's not a getter. Getters shouldn't do lots of processing! | |||
There was a problem hiding this comment.
Nit: TODO: instead of TODO - :)
There was a problem hiding this comment.
Hi - I really don't want to spend the 15 minutes on context switching to remove a '-'. Can this be accepted as is (assuming no other issues)?
|
Is this waiting on me for anything? |
|
@jfstephe No, it's waiting until we have setup the branch for beta releases and then we'll merge. I'll fix the |
|
@devoto13 - Hi, it's been almost a year on this. Any eta on this being merged? |
|
@jfstephe I don't have permissions to merge, so without somebody from Google, there is nothing I can do. And given the recent karma deprecation announcement, I'm afraid that the most likely eta is never, sorry 😞 |
When running karma in watch mode, and multiple files change (triggered for example by this typescript compiler issue), then the total time for the change handlers to complete is VERY slow - 100ms approx, per file on my machine. When all the files in the project are touched this causes a massive delay on the test execution.
The performance issue seems to be related to a getter that takes a long time to run. I think this is only returning things for test purposes ATM, so this PR removes this.