-
Notifications
You must be signed in to change notification settings - Fork 113
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
Update: Add BoundedCache for Thumbnails Sidebar #917
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
import Cache from './Cache'; | ||
|
||
class BoundedCache extends Cache { | ||
/** @property {Array} - Maintains the list of cache keys in order in which they were added to the cache */ | ||
cacheQueue; | ||
|
||
/** @property {number} - The maximum number of entries in the cache */ | ||
maxEntries; | ||
|
||
/** | ||
* [constructor] | ||
* | ||
* @param {number} [maxEntries] - Override the maximum number of cache entries | ||
*/ | ||
constructor(maxEntries) { | ||
super(); | ||
|
||
this.maxEntries = maxEntries || 500; | ||
this.cache = {}; | ||
this.cacheQueue = []; | ||
} | ||
|
||
/** | ||
* Destroys the bounded cache | ||
* | ||
* @return {void} | ||
*/ | ||
destroy() { | ||
this.cache = null; | ||
this.cacheQueue = null; | ||
} | ||
|
||
/** | ||
* Caches a simple object in memory. If the number of cache entries | ||
* then exceeds the maxEntries value, then the earliest key in cacheQueue | ||
* will be removed from the cache. | ||
* | ||
* @param {string} key - The cache key | ||
* @param {*} value - The cache value | ||
* @return {void} | ||
*/ | ||
set(key, value) { | ||
// If this key is not already in the cache, then add it | ||
// to the cacheQueue. This avoids adding the same key to | ||
// the cacheQueue multiple times if the cache entry gets updated | ||
if (!this.inCache(key)) { | ||
this.cacheQueue.push(key); | ||
} | ||
|
||
super.set(key, value); | ||
|
||
// If the cacheQueue exceeds the maxEntries then remove the first | ||
// key from the front of the cacheQueue and unset that entry | ||
// from the cache | ||
if (this.cacheQueue.length > this.maxEntries) { | ||
const deleteKey = this.cacheQueue.shift(); | ||
this.unset(deleteKey); | ||
} | ||
} | ||
} | ||
|
||
export default BoundedCache; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import isFinite from 'lodash/isFinite'; | ||
import VirtualScroller from './VirtualScroller'; | ||
import { CLASS_HIDDEN } from './constants'; | ||
import BoundedCache from './BoundedCache'; | ||
|
||
const CLASS_BOX_PREVIEW_THUMBNAIL = 'bp-thumbnail'; | ||
const CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE = 'bp-thumbnail-image'; | ||
|
@@ -43,7 +44,7 @@ class ThumbnailsSidebar { | |
this.anchorEl = element; | ||
this.currentThumbnails = []; | ||
this.pdfViewer = pdfViewer; | ||
this.thumbnailImageCache = {}; | ||
this.thumbnailImageCache = new BoundedCache(); | ||
|
||
this.createImageEl = this.createImageEl.bind(this); | ||
this.createPlaceholderThumbnail = this.createPlaceholderThumbnail.bind(this); | ||
|
@@ -94,7 +95,11 @@ class ThumbnailsSidebar { | |
this.virtualScroller = null; | ||
} | ||
|
||
this.thumbnailImageCache = null; | ||
if (this.thumbnailImageCache) { | ||
this.thumbnailImageCache.destroy(); | ||
this.thumbnailImageCache = null; | ||
} | ||
|
||
this.pdfViewer = null; | ||
this.currentThumbnails = []; | ||
this.currentPage = null; | ||
|
@@ -201,6 +206,14 @@ class ThumbnailsSidebar { | |
thumbnailEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAIL_IS_SELECTED); | ||
} | ||
|
||
// If image is already in cache, then use it instead of waiting for | ||
// the second render image pass | ||
const cachedImage = this.thumbnailImageCache.get(itemIndex); | ||
if (cachedImage && !cachedImage.inProgress) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't we still want to use it even if it's in progress again? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if It's in progress, that means there is no generated image yet so there's nothing to insert into the DOM at this point. We add a cache entry with |
||
thumbnailEl.appendChild(cachedImage.image); | ||
thumbnailEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAIL_IMAGE_LOADED); | ||
} | ||
|
||
return thumbnailEl; | ||
} | ||
|
||
|
@@ -233,7 +246,7 @@ class ThumbnailsSidebar { | |
* @return {Promise} - promise reolves with the image HTMLElement or null if generation is in progress | ||
*/ | ||
createThumbnailImage(itemIndex) { | ||
const cacheEntry = this.thumbnailImageCache[itemIndex]; | ||
const cacheEntry = this.thumbnailImageCache.get(itemIndex); | ||
|
||
// If this thumbnail has already been cached, use it | ||
if (cacheEntry && cacheEntry.image) { | ||
|
@@ -246,13 +259,13 @@ class ThumbnailsSidebar { | |
} | ||
|
||
// Update the cache entry to be in progress | ||
this.thumbnailImageCache[itemIndex] = { ...cacheEntry, inProgress: true }; | ||
this.thumbnailImageCache.set(itemIndex, { ...cacheEntry, inProgress: true }); | ||
|
||
return this.getThumbnailDataURL(itemIndex + 1) | ||
.then(this.createImageEl) | ||
.then((imageEl) => { | ||
// Cache this image element for future use | ||
this.thumbnailImageCache[itemIndex] = { inProgress: false, image: imageEl }; | ||
this.thumbnailImageCache.set(itemIndex, { inProgress: false, image: imageEl }); | ||
|
||
return imageEl; | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
/* eslint-disable no-unused-expressions */ | ||
import BoundedCache from '../BoundedCache'; | ||
|
||
const sandbox = sinon.sandbox.create(); | ||
|
||
describe('BoundedCache', () => { | ||
let cache; | ||
|
||
beforeEach(() => { | ||
cache = new BoundedCache(2); | ||
}); | ||
|
||
afterEach(() => { | ||
sandbox.verifyAndRestore(); | ||
|
||
cache = null; | ||
}); | ||
|
||
describe('constructor()', () => { | ||
it('should initialize properties', () => { | ||
cache = new BoundedCache(); | ||
|
||
expect(cache.maxEntries).to.be.equal(500); | ||
expect(cache.cache).to.be.empty; | ||
expect(cache.cacheQueue.length).to.be.equal(0); | ||
}); | ||
|
||
it('should handle maxEntries', () => { | ||
expect(cache.maxEntries).to.be.equal(2); | ||
}); | ||
}); | ||
|
||
describe('set()', () => { | ||
it('should add the entry to the cache', () => { | ||
cache.set('foo', 'bar'); | ||
|
||
expect(cache.inCache('foo')).to.be.true; | ||
expect(cache.cacheQueue).to.be.eql(['foo']); | ||
}); | ||
|
||
it('should not update the cacheQueue if key already exists', () => { | ||
cache.set('foo', 'bar'); | ||
cache.set('foo', 'bar2'); | ||
|
||
expect(cache.inCache('foo')).to.be.true; | ||
expect(cache.get('foo')).to.be.equal('bar2'); | ||
expect(cache.cacheQueue).to.be.eql(['foo']); | ||
}); | ||
|
||
it('should remove the earliest added entry when entries exceed maxEntries', () => { | ||
cache.set('foo', 'bar'); | ||
cache.set('hello', 'world'); | ||
cache.set('goodnight', 'moon'); | ||
|
||
expect(cache.inCache('foo')).to.be.false; | ||
expect(cache.cacheQueue).to.be.eql(['hello', 'goodnight']); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 500?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is more specific to the application of Thumbnail images, but it's basically arbitrary. I figure if the images are ~50KB 500 would be 25MB of cache which seemed like an ok starting point