Store filesystem stats on tree-view entries #859

Alhadis · 2016-06-03T07:01:56Z

Developers may need to access more granular data when traversing the tree-view's contents... a file's modification time, its filesize, permissions, and so forth. All of this data is already available to the tree-view as it's traversing the filesystem, but only a narrow subset of it is exposed to the File and Directory instances it creates. This PR amends the constructor to include the "pure" filesystem stats on an instanced property, .stats.

Implementation

Rather than passing a direct reference to the value returned by lstatSyncNoException, I've flattened its instanced properties into a new object, cutting out any filesystem methods. This was done out of a (perhaps excessive) concern for performance - I didn't want to keep anything from being garbage collected, and I'm unsure how Node goes about maintaining filesystem connections under the hood.

Performance benefits

Aside from convenience, a boost to performance is gained by circumventing the need for excess filesystem calls. For instance, I used the event emission callback pending in #833 to run a simplistic benchmark:

Without .stats With .stats

start = window.performance.now()
statCache = {}

grabStats = (from) ->

{path} = from

stat = fs.lstatSyncNoException(path)

symlink = stat.isSymbolicLink?()

if symlink

stat = fs.statSyncNoException(path)

statCache[path] = stat
grabStats dir

for path, file of dir.entries

grabStats file
end = window.performance.now()

"Time: #{end - start}"

start = window.performance.now()
statCache = {}

grabStats = (from) ->

statCache[path] = from.stats
grabStats dir

for path, file of dir.entries

grabStats file
end = window.performance.now()

"Time: #{end - start}"

Time: 3.5 Time: 0.37

The times shown above are averages (trailing digits clipped to a sane length), tested against a folder holding 632 files (most of them blank). This was run on fairly modern hardware (MacBook Pro / Mavericks 10.9.5 / Processor: 2.3 GHz Intel Core i7 / Memory: 16 GBs 1600 MHz DDR3). While the reduction shown above isn't exactly thrilling, scalability should always be considered.

Developers monitoring the tree-view may need to access file-system stats for an entry. Having the stats available on the instance saves an author from making another filesystem call. For very large directories, this is a non-trivial boost to performance. Rather than pass a reference to the stat variable directly, a new object is created to give to the Directory or File instance. This might benefit garbage collection, although this is based on little more than theory.

Alhadis · 2016-06-03T17:23:49Z

@50Wliu Spare a little advice? I noticed Travis failed the build, but that's the same odd error I got at sporadic intervals when running the package specs. It only appeared after reloading the package the first time... whenever I ran it after that, everything passed.

Have you ever encountered anything like that before when running specs...?

winstliu · 2016-06-03T17:27:58Z

I haven't seen that spec fail before, but it also seems like a recent addition to the suite. Sorry I can't help you further.

Alhadis · 2016-06-03T17:29:02Z

S'alright. =) I'll see if I can work it out tomorrow.

Alhadis · 2016-06-04T17:14:16Z

Okay, I think I managed to fix the failing specs. Based on the notes @BinaryMuse left in d928555, I deduced the failure might've been connected to the use of .click() to open the directory in one of the tests. I used .expand()[0] instead, but because of the error's unpredictable nature, I've no way of knowing if this actually fixed it for good.

This allows easier comparison of times, and also stops Date objects from being serialised into strings when passed through Task handlers.

thomasjo · 2016-06-05T08:20:58Z

lib/directory.coffee

@@ -177,6 +177,9 @@ class Directory
      stat = fs.lstatSyncNoException(fullPath)
      symlink = stat.isSymbolicLink?()
      stat = fs.statSyncNoException(fullPath) if symlink
+      statFlat = _.pick stat, _.keys(stat)...
+      for i in ["atime", "birthtime", "ctime", "mtime"]


Minor thing, but this is not an index, so I think the i should be changed to key, property, type, or some other colloquialism that is more appropriate than i.

Changed.

(FWIW, I usually mean iteration when I write i).

@thomasjo

Requested by @thomasjo: - #859 (diff)

If a date-typed stat is undefined, avoid throwing a TypeError.

Alhadis · 2016-06-06T09:05:13Z

@BinaryMuse Would you be able to explain why the build might be failing? It's stopping at a test you added:

TreeView
  when the package first activates and there is a file open (regression)
    when the file is pending
      it marks the pending file as permanent
        Expected null to be <TextEditor 176>. (spec/tree-view-spec.coffee:594:67)

This was working before, and I added a null-coalescing operator to prevent breakage for undefined properties (which is entirely unrelated). The build's suddenly failing again, so I'm not sure what might be causing this.

If it helps, I have the pending file feature disabled, but I don't think user preferences have anything to do with running specs.

Alhadis · 2016-07-16T05:40:18Z

Another example of a package that could benefit from this.

Alhadis · 2016-07-24T10:04:25Z

@50Wliu AppVeyor builds appear to be failing all over the place over the last couple days...

Error processing argument at index 9, conversion failure from 
TypeError: Error processing argument at index 9, conversion failure from 
    at TypeError (native)
    at Object.module.exports.showMessageBox (C:\projects\tree-view\Atom\resources\atom.asar\browser\api\lib\dialog.js:155:20)
    at EventEmitter.<anonymous> (C:\projects\tree-view\Atom\resources\app.asar\src\browser\atom-window.js:221:27)
    at emitOne (events.js:82:20)
    at EventEmitter.emit (events.js:169:7)
Specs Failed

Other failures

Those last two also have a tonne of build errors, none of which appear to be related. :S This is clearly an issue with AppVeyor, but I'm not sure where to begin reporting it... so I'm pinging you (please let me know if there's a better place to bring this up if it happens again in future).

winstliu · 2016-07-24T14:37:30Z

See #791. tree-view specs currently do not run on Windows yet, so AppVeyor can be safely ignored.

In addition that other failing spec you mentioned on June 6 appears to be flaky even on master.

Alhadis · 2016-07-24T14:41:22Z

Ah right, thanks! That's good to know. =)

In addition that other failing spec you mentioned on June 6 appears to be flaky even on master.

So we're dealing with a Heisenbug? Great... :S :\

Alhadis · 2016-08-09T08:01:08Z

@as-cii Nagging you because I feel you're the one most likely to respond first.

as-cii · 2016-08-09T08:13:13Z

Thanks, @Alhadis!

Alhadis · 2016-08-09T08:23:55Z

NO, THANK YOU.

Alhadis added 2 commits June 3, 2016 14:03

Add specs for da97a26

8bde288

winstliu added performance needs-review labels Jun 3, 2016

Use expand method to open directories in spec

ca9acf6

Alhadis added 2 commits June 5, 2016 16:43

Convert date-based stats to Unix timestamps

3aff76c

This allows easier comparison of times, and also stops Date objects from being serialised into strings when passed through Task handlers.

Remove console.log call from Directory class

023ac75

thomasjo reviewed Jun 5, 2016
View reviewed changes

Alhadis added 2 commits June 5, 2016 18:31

Use a more specific name for iterator variables

6980dfc

Requested by @thomasjo: - #859 (diff)

Avoid breaking on non-existent filesystem times

61adcbc

If a date-typed stat is undefined, avoid throwing a TypeError.

Isolate filesystem-stats spec to a new file

d82ce95

Merge branch "master" into entry-stats

3a30133

as-cii merged commit 46076ca into atom:master Aug 9, 2016

Alhadis deleted the entry-stats branch August 9, 2016 08:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store filesystem stats on tree-view entries #859

Store filesystem stats on tree-view entries #859

Alhadis commented Jun 3, 2016

Alhadis commented Jun 3, 2016

winstliu commented Jun 3, 2016 •

edited

Loading

Alhadis commented Jun 3, 2016

Alhadis commented Jun 4, 2016 •

edited

Loading

thomasjo Jun 5, 2016 •

edited

Loading

Alhadis Jun 5, 2016

Alhadis commented Jun 6, 2016 •

edited

Loading

Alhadis commented Jul 16, 2016

Alhadis commented Jul 24, 2016 •

edited

Loading

winstliu commented Jul 24, 2016

Alhadis commented Jul 24, 2016

Alhadis commented Aug 9, 2016

as-cii commented Aug 9, 2016

Alhadis commented Aug 9, 2016

Store filesystem stats on tree-view entries #859

Store filesystem stats on tree-view entries #859

Conversation

Alhadis commented Jun 3, 2016

Implementation

Performance benefits

Alhadis commented Jun 3, 2016

winstliu commented Jun 3, 2016 • edited Loading

Alhadis commented Jun 3, 2016

Alhadis commented Jun 4, 2016 • edited Loading

thomasjo Jun 5, 2016 • edited Loading

Choose a reason for hiding this comment

Alhadis Jun 5, 2016

Choose a reason for hiding this comment

Alhadis commented Jun 6, 2016 • edited Loading

Alhadis commented Jul 16, 2016

Alhadis commented Jul 24, 2016 • edited Loading

winstliu commented Jul 24, 2016

Alhadis commented Jul 24, 2016

Alhadis commented Aug 9, 2016

as-cii commented Aug 9, 2016

Alhadis commented Aug 9, 2016

winstliu commented Jun 3, 2016 •

edited

Loading

Alhadis commented Jun 4, 2016 •

edited

Loading

thomasjo Jun 5, 2016 •

edited

Loading

Alhadis commented Jun 6, 2016 •

edited

Loading

Alhadis commented Jul 24, 2016 •

edited

Loading