Skip to content

Commit

Permalink
perf(watcher): Significantly improved addFile/changeFile/removeFile p…
Browse files Browse the repository at this point in the history
…erformance.

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.
  • Loading branch information
jfstephe authored and jfstephe committed Nov 15, 2021
1 parent b153355 commit 57740d6
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 22 deletions.
13 changes: 5 additions & 8 deletions lib/file-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -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!
get files () {
const served = []
const included = {}
Expand Down Expand Up @@ -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
}

const pattern = this._findIncluded(path)
if (!pattern) {
log.debug(`Add file "${path}" ignored. Does not match any pattern.`)
return this.files
return
}

if (this._exists(path)) {
log.debug(`Add file "${path}" ignored. Already in the list.`)
return this.files
return
}

const file = new File(path)
Expand All @@ -195,7 +196,6 @@ class FileList {

log.info(`Added file "${path}".`)
this._emitModified()
return this.files
}

async changeFile (path, force) {
Expand All @@ -204,7 +204,7 @@ class FileList {

if (!file) {
log.debug(`Changed file "${path}" ignored. Does not match any file in the list.`)
return this.files
return
}

const [stat] = await Promise.all([statAsync(path), this._refreshing])
Expand All @@ -214,7 +214,6 @@ class FileList {
log.info(`Changed file "${path}".`)
this._emitModified(force)
}
return this.files
}

async removeFile (path) {
Expand All @@ -224,12 +223,10 @@ class FileList {
if (file) {
helper.arrayRemove(this._getFilesByPattern(pattern.pattern), file)
log.info(`Removed file "${path}".`)

this._emitModified()
} else {
log.debug(`Removed file "${path}" ignored. Does not match any file in the list.`)
}
return this.files
}
}

Expand Down
28 changes: 14 additions & 14 deletions test/unit/file-list.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -464,32 +464,32 @@ describe('FileList', () => {

it('does not add excluded files', () => {
return list.refresh().then((before) => {
return list.addFile('/secret/hello.txt').then((files) => {
expect(files.served).to.be.eql(before.served)
return list.addFile('/secret/hello.txt').then(() => {
expect(list.files.served).to.be.eql(before.served)
})
})
})

it('does not add already existing files', () => {
return list.refresh().then((before) => {
return list.addFile('/some/a.js').then((files) => {
expect(files.served).to.be.eql(before.served)
return list.addFile('/some/a.js').then(() => {
expect(list.files.served).to.be.eql(before.served)
})
})
})

it('does not add unmatching files', () => {
return list.refresh().then((before) => {
return list.addFile('/some/a.ex').then((files) => {
expect(files.served).to.be.eql(before.served)
return list.addFile('/some/a.ex').then(() => {
expect(list.files.served).to.be.eql(before.served)
})
})
})

it('adds the file to the correct bucket', () => {
return list.refresh().then((before) => {
return list.addFile('/some/d.js').then((files) => {
expect(pathsFrom(files.served)).to.contain('/some/d.js')
return list.addFile('/some/d.js').then(() => {
expect(pathsFrom(list.files.served)).to.contain('/some/d.js')
const bucket = list.buckets.get('/some/*.js')
expect(pathsFrom(bucket)).to.contain('/some/d.js')
})
Expand Down Expand Up @@ -531,8 +531,8 @@ describe('FileList', () => {
list = new List(patterns('/a.*'), [], emitter, preprocess)

return list.refresh().then(() => {
return list.addFile('/a.js').then((files) => {
expect(findFile('/a.js', files.served).mtime).to.eql(new Date('2012-01-01'))
return list.addFile('/a.js').then(() => {
expect(findFile('/a.js', list.files.served).mtime).to.eql(new Date('2012-01-01'))
})
})
})
Expand Down Expand Up @@ -597,10 +597,10 @@ describe('FileList', () => {
mockFs._touchFile('/some/b.js', '3020-01-01')
modified.resetHistory()

return list.changeFile('/some/b.js').then((files) => {
return list.changeFile('/some/b.js').then(() => {
clock.tick(101)
expect(modified).to.have.been.calledOnce
expect(findFile('/some/b.js', files.served).mtime).to.be.eql(new Date('3020-01-01'))
expect(findFile('/some/b.js', list.files.served).mtime).to.be.eql(new Date('3020-01-01'))
})
})
})
Expand Down Expand Up @@ -719,8 +719,8 @@ describe('FileList', () => {
return list.refresh().then((files) => {
modified.resetHistory()
return list.removeFile('/some/a.js')
}).then((files) => {
expect(pathsFrom(files.served)).to.be.eql([
}).then(() => {
expect(pathsFrom(list.files.served)).to.be.eql([
'/some/b.js',
'/a.txt'
])
Expand Down

0 comments on commit 57740d6

Please sign in to comment.