improvements to file system events processing for search #12885

zaggino · 2016-11-09T05:16:58Z

Hi guys, I've reported elsewhere that my db dump script crashes both brackets (white-screen) and also brackets-electron. This is the work done to fix that (and I can confirm that with this fix there's no crash when running my script anymore) - it's not final and I'd like to have it reviewed before working on little things as docs and similar.

There're two main things, first aggregate duplicate events and debounce for a short while before processing. My script quickly deletes and creates back a lot of stuff, before this change some of the directories were removed and added back to search multiple times. With debouncing it's only once after script is done.

Second, no less important thing is fixing calls to filesChanged and filesRemoved functions. In case of my script these two were called about 1000 times with an array of size 1 which cause a lot of trouble for IPC connection in electron and socket connection in regular brackets. With the fix these are now properly called.

Marking this as high-priority because it fixes brackets completely crashing in some cases.

petetnt

Added some comments.

petetnt · 2016-11-09T21:13:48Z

src/search/FindInFiles.js

@@ -657,8 +665,8 @@ define(function (require, exports, module) {
     * Inform node that the list of files has changed.
     * @param {array} fileList The list of files that changed.
     */
-    function filesChanged(fileList) {
-        if (FindUtils.isNodeSearchDisabled() || fileList.length === 0) {
+    function filesChanged(fileList) {        


Some trailing whitespace here

petetnt · 2016-11-09T21:14:31Z

src/search/FindInFiles.js

-    function filesChanged(fileList) {
-        if (FindUtils.isNodeSearchDisabled() || fileList.length === 0) {
+    function filesChanged(fileList) {        
+        if (FindUtils.isNodeSearchDisabled() || !fileList || fileList.length === 0) {


!fileList covers fileList.length === 0 too.

no it doesn't

var emptyArray = []; var a = !emptyArray; var b = emptyArray.length === 0; [a,b]; [false, true]

Ugh, sorry about that @zaggino, I automatically assumed that fileList would never be an empty array just undefined. Again, sorry.

Leaving it as is, it's a very cheap check and better be safe than sorry :)

petetnt · 2016-11-09T21:25:47Z

src/search/FindInFiles.js

+                        // node search : inform node that the file is removed
+                        fullPaths.push(fullPath);
+                        if (findOrReplaceInProgress) {
+                            searchModel.removeResults(fullPath);


Should this be called once with a large array too?

removeResults is a classic sync method, so it wouldn't do any benefit here

zaggino@ Bro how are you today? Please Bro help us we want CoffeeScript in Brakets and also
Babel , and many's other please

petetnt · 2016-11-09T21:28:26Z

src/search/FindInFiles.js

-                        resultsChanged = true;
+        function _removeSearchResultsForEntries(entries) {
+            var fullPaths = [];
+            entries.forEach(function (entry) {


Might be worth checking out if couple of Array.prototype.maps would be faster than nested forEachs and a push

fullPaths.push(fullPath); is inside an if so if map would be used, we'd need to filter it out for nulls afterwards

Valid point 👍

petetnt · 2016-11-09T22:46:22Z

Should avoid doing code reviews when I am tired 😴 👍

Nice work @zaggino

ficristo · 2016-11-12T09:04:51Z

src/search/FindInFiles.js

+        clearSearch;
+
+    /* Waits for FS changes to stack up until processing them (scripts like npm install can do a lot of movements on the disk) */
+    var FILE_SYSTEM_EVENT_DEBOUNCE_TIME = 1000;


Can this be something like 100ms instead? Or is too low?

You're right here, re-tested this set to 100ms with my script and it's fine. So setting it to 100ms.

ficristo · 2016-11-12T09:05:52Z

src/search/FindInFiles.js

-                            addedFiles.push(child);
-                            addedFilePaths.push(child.fullPath);
+        function _addSearchResultsForEntries(entries) {
+            var fullPaths = [];


Do you have a rough idea of the memory usage here? If we store a lot of path could cause some trouble?

Well, this array is not being persisted besides the function execution lifetime and I wouldn't expect there to be a problem even if you have thousands of entries in it. On the other hand, without it, trying to send thousands of messages to a node process will definitely crash it (or socket server). In other words, I don't think memory usage is something to worry about.

ficristo · 2016-11-12T09:09:13Z

src/search/FindInFiles.js

+    }, FILE_SYSTEM_EVENT_DEBOUNCE_TIME);
+
+    _debouncedFileSystemChangeHandler = function (event, entry, added, removed) {
+        _cachedFileSystemEvents.push([ event, entry, added, removed ]);


Could you push an object instead of an array? So I will not feel so dumb when reading the reduce function above...

Sure, can do.

ficristo · 2016-11-12T09:09:43Z

Nice work @zaggino

👍

zaggino · 2016-11-14T03:16:10Z

Pushed some changes, @petetnt @ficristo please review again :-)

ficristo · 2016-11-14T18:50:39Z

src/search/FindInFiles.js

+        _.forEach(_cachedFileSystemEvents, function (obj) {
+            _fileSystemChangeHandler(obj.event, obj.entry, obj.added, obj.removed);
+        });
+        _cachedFileSystemEvents = [];


Since we are accessing the same global, and this function is called with a delay, don't we risk here to delete the events that have been added in the bouncedFileSystemChangeHandler function while waiting this to run?

How exactly? There's a _.forEach above this that launches _fileSystemChangeHandler for every object in the array, before deleting it. The function is completely synchronous - if an fs event is emitted while this is running, it'll be added to the empty array once this function finishes.

Probably I was overthinking, but I was wondering if there could be some races between _debouncedFileSystemChangeHandler and _processCachedFileSystemEvents.

No, races certainly can't happen in this case. They are both completely sync functions - no promises or async-await there.

The debounce and relative timout are threwing me off.

debounce just wraps a function and keeps postponing its execution until the wrapper stops to be called for the timeout time. But once the wrapped function is executed, all happens totally sync so the code is safe. I've used this pattern a few times before.

ficristo · 2016-11-15T06:28:51Z

src/search/FindInFiles.js

+    _processCachedFileSystemEvents = _.debounce(function () {
+        // we need to reduce _cachedFileSystemEvents not to contain duplicates!
+        _cachedFileSystemEvents = _cachedFileSystemEvents.reduce(function (result, obj) {
+            var fullPath = obj.entry ? obj.entry.fullPath : null;


If fullPath is null maybe return immediately?

Deeper in the code there's a handle for this case (doing a full refresh I think) so it didn't want to stop it here.

So the assignment below is safe: result[fullPath] = obj; ?

Yes, it actuall assigns result[null] = obj; which is a bit weird but valid so I left it like that.

zaggino · 2016-11-17T00:49:59Z

@petetnt anything else? if not it should be good to merge

petetnt · 2016-11-17T08:45:21Z

@zaggino LGTM, I'll run this in production use for today and merge it in if nothing pops up.

swmitra

LGTM

petetnt · 2016-11-17T16:41:05Z

Used it all day without any issues and Brackets did feel snappier. Great job once again @zaggino!

improvements to file system events processing

500b11d

zaggino added the high priority label Nov 9, 2016

petetnt reviewed Nov 9, 2016

View reviewed changes

ficristo reviewed Nov 12, 2016

View reviewed changes

zaggino added 2 commits November 14, 2016 14:00

working on comments

a8567f4

jsdoc

cb58286

little bit of optimization

69c3243

ficristo reviewed Nov 14, 2016

View reviewed changes

ficristo reviewed Nov 15, 2016

View reviewed changes

ficristo approved these changes Nov 15, 2016

View reviewed changes

zaggino added this to the Release 1.9 milestone Nov 15, 2016

swmitra approved these changes Nov 17, 2016

View reviewed changes

petetnt approved these changes Nov 17, 2016

View reviewed changes

petetnt merged commit 94ed8b3 into master Nov 17, 2016

petetnt deleted the zaggino/fs-improvements branch November 17, 2016 16:41

zaggino mentioned this pull request Nov 17, 2016

improvements to file system events processing brackets-userland/brackets-electron#80

Closed

ficristo mentioned this pull request Nov 28, 2016

Fix FindInFiles tests #12951

Closed

This was referenced Aug 30, 2021

[CLOSED] improvements to file system events processing for search brackets-archive/bracketsIssues#10970

Open

[CLOSED] Fix FindInFiles tests brackets-archive/bracketsIssues#11019

Open

improvements to file system events processing for search #12885

improvements to file system events processing for search #12885

Conversation

zaggino commented Nov 9, 2016

petetnt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petetnt commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo Nov 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ficristo commented Nov 12, 2016

zaggino commented Nov 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaggino commented Nov 17, 2016

petetnt commented Nov 17, 2016

swmitra left a comment

Choose a reason for hiding this comment

petetnt commented Nov 17, 2016

ficristo Nov 12, 2016 •

edited

Loading