From 920e7575fadd6425ba4af08a5bf3275819ca7d50 Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Mon, 17 Aug 2015 16:23:12 -0700 Subject: [PATCH] perf(drafts): Cache results of running Autolinker, avoid props.children for EmailFrame, fix special case for new draft Summary: "results.length" not "result.length". Eventually I want MessageBodyProcessor to be the public interface to another process which asynchronously runs message body parsing? At the very least, caching the results means that miscelaneous refreshes to the message list don't incur significant string processing cost. Test Plan: No new tests here Reviewers: evan Reviewed By: evan Differential Revision: https://phab.nylas.com/D1898 --- exports/nylas-exports.coffee | 1 + .../message-list/lib/email-frame.cjsx | 9 +-- .../message-list/lib/message-item.cjsx | 35 ++--------- .../message-list/spec/message-item-spec.cjsx | 5 +- .../lib/thread-list-quick-actions.cjsx | 2 +- src/browser/application.coffee | 26 +++++++- src/flux/stores/database-store.coffee | 2 +- src/flux/stores/message-body-processor.coffee | 60 +++++++++++++++++++ src/flux/stores/message-store.coffee | 15 ++++- src/flux/tasks/destroy-draft.coffee | 2 +- 10 files changed, 113 insertions(+), 44 deletions(-) create mode 100644 src/flux/stores/message-body-processor.coffee diff --git a/exports/nylas-exports.coffee b/exports/nylas-exports.coffee index 4efe6ef66..812de4c56 100644 --- a/exports/nylas-exports.coffee +++ b/exports/nylas-exports.coffee @@ -62,6 +62,7 @@ Exports = DraftCountStore: require '../src/flux/stores/draft-count-store' DraftStoreExtension: require '../src/flux/stores/draft-store-extension' MessageStore: require '../src/flux/stores/message-store' + MessageBodyProcessor: require '../src/flux/stores/message-body-processor' EventStore: require '../src/flux/stores/event-store' MessageStoreExtension: require '../src/flux/stores/message-store-extension' ContactStore: require '../src/flux/stores/contact-store' diff --git a/internal_packages/message-list/lib/email-frame.cjsx b/internal_packages/message-list/lib/email-frame.cjsx index a6b3afed6..54c9c41a5 100644 --- a/internal_packages/message-list/lib/email-frame.cjsx +++ b/internal_packages/message-list/lib/email-frame.cjsx @@ -6,6 +6,9 @@ _ = require "underscore" class EmailFrame extends React.Component @displayName = 'EmailFrame' + @propTypes: + content: React.PropTypes.string.isRequired + render: => @@ -57,13 +60,11 @@ class EmailFrame extends React.Component @_setFrameHeight() _emailContent: => - email = @props.children - # When showing quoted text, always return the pure content if @props.showQuotedText - email + @props.content else - QuotedHTMLParser.hideQuotedHTML(email) + QuotedHTMLParser.hideQuotedHTML(@props.content) module.exports = EmailFrame diff --git a/internal_packages/message-list/lib/message-item.cjsx b/internal_packages/message-list/lib/message-item.cjsx index b76aecda2..64ed7d309 100644 --- a/internal_packages/message-list/lib/message-item.cjsx +++ b/internal_packages/message-list/lib/message-item.cjsx @@ -10,6 +10,7 @@ MessageControls = require './message-controls' MessageUtils, NamespaceStore, MessageStore, + MessageBodyProcessor, QuotedHTMLParser, ComponentRegistry, FileDownloadStore} = require 'nylas-exports' @@ -18,7 +19,6 @@ MessageControls = require './message-controls' InjectedComponent} = require 'nylas-component-kit' TransparentPixel = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR4nGNikAQAACIAHF/uBd8AAAAASUVORK5CYII=" -MessageBodyWidth = 740 class MessageItem extends React.Component @displayName = 'MessageItem' @@ -79,9 +79,7 @@ class MessageItem extends React.Component
{@_renderHeader()} - - {@_formatBody()} - + {@_renderEvents()} {@_renderAttachments()} @@ -206,33 +204,8 @@ class MessageItem extends React.Component _formatBody: => return "" unless @props.message and @props.message.body - # Give each extension the message object to process the body, but don't - # allow them to modify anything but the body for the time being. - body = @props.message.body - for extension in MessageStore.extensions() - continue unless extension.formatMessageBody - virtual = @props.message.clone() - virtual.body = body - extension.formatMessageBody(virtual) - body = virtual.body - - # Find inline images and give them a calculated CSS height based on - # html width and height, when available. This means nothing changes size - # as the image is loaded, and we can estimate final height correctly. - # Note that MessageBodyWidth must be updated if the UI is changed! - - while (result = MessageUtils.cidRegex.exec(body)) isnt null - imgstart = body.lastIndexOf('<', result.index) - imgend = body.indexOf('/>', result.index) - - if imgstart != -1 and imgend > imgstart - imgtag = body.substr(imgstart, imgend - imgstart) - width = imgtag.match(/width[ ]?=[ ]?['"]?(\d*)['"]?/)?[1] - height = imgtag.match(/height[ ]?=[ ]?['"]?(\d*)['"]?/)?[1] - if width and height - scale = Math.min(1, MessageBodyWidth / width) - style = " style=\"height:#{height * scale}px;\" " - body = body.substr(0, imgend) + style + body.substr(imgend) + # Runs extensions, potentially asynchronous soon + body = MessageBodyProcessor.process(@props.message.id, @props.message) # Replace cid:// references with the paths to downloaded files for file in @props.message.files diff --git a/internal_packages/message-list/spec/message-item-spec.cjsx b/internal_packages/message-list/spec/message-item-spec.cjsx index 5ee1d6334..965e4941c 100644 --- a/internal_packages/message-list/spec/message-item-spec.cjsx +++ b/internal_packages/message-list/spec/message-item-spec.cjsx @@ -8,7 +8,8 @@ ReactTestUtils = React.addons.TestUtils Thread, Utils, QuotedHTMLParser, - FileDownloadStore} = require "nylas-exports" + FileDownloadStore, + MessageBodyProcessor} = require "nylas-exports" EmailFrameStub = React.createClass({render: ->
}) @@ -97,6 +98,8 @@ describe "MessageItem", -> spyOn(FileDownloadStore, 'downloadDataForFiles').andCallFake (ids) -> return {'file_1_id': download, 'file_inline_downloading_id': download_inline} + spyOn(MessageBodyProcessor, 'addToCache').andCallFake -> + @message = new Message id: "111" from: [user_1] diff --git a/internal_packages/thread-list/lib/thread-list-quick-actions.cjsx b/internal_packages/thread-list/lib/thread-list-quick-actions.cjsx index cf9a5b1e6..6daef3a07 100644 --- a/internal_packages/thread-list/lib/thread-list-quick-actions.cjsx +++ b/internal_packages/thread-list/lib/thread-list-quick-actions.cjsx @@ -13,7 +13,7 @@ class ThreadListQuickActions extends React.Component @displayName: 'ThreadListQuickActions' @propTypes: thread: React.PropTypes.object - categoryId: React.PropTypes.integer + categoryId: React.PropTypes.string render: => actions = [] diff --git a/src/browser/application.coffee b/src/browser/application.coffee index b3e30d656..a4ca7b8fa 100644 --- a/src/browser/application.coffee +++ b/src/browser/application.coffee @@ -123,6 +123,28 @@ class Application # which is why this check is here. throw error unless error.code is 'ENOENT' + # On Windows, removing a file can fail if a process still has it open. When + # we close windows and log out, we need to wait for these processes to completely + # exit and then delete the file. It's hard to tell when this happens, so we just + # retry the deletion a few times. + deleteFileWithRetry: (filePath, callback, retries = 5) -> + callbackWithRetry = (err) => + if err + console.log("File Error: #{err.message} - retrying in 150msec") + setTimeout => + @deleteFileWithRetry(filePath, callback, retries - 1) + , 150 + else + callback(null) + + if not fs.existsSync(filePath) + callback(null) + + if retries > 0 + fs.unlink(filePath, callbackWithRetry) + else + fs.unlink(filePath, callback) + # Configures required javascript environment flags. setupJavaScriptArguments: -> app.commandLine.appendSwitch 'js-flags', '--harmony' @@ -131,7 +153,7 @@ class Application @setDatabasePhase('close') @windowManager.closeMainWindow() @windowManager.unregisterAllHotWindows() - fs.unlink path.join(configDirPath,'edgehill.db'), => + @deleteFileWithRetry path.join(configDirPath,'edgehill.db'), => @config.set('nylas', null) @config.set('edgehill', null) @setDatabasePhase('setup') @@ -156,7 +178,7 @@ class Application message: 'Upgrading Nylas' detail: 'Welcome back to Nylas! We need to rebuild your mailbox to support new features. Please wait a few moments while we re-sync your mail.' buttons: ['OK'] - fs.unlink path.join(configDirPath,'edgehill.db'), (err) => + @deleteFileWithRetry path.join(configDirPath,'edgehill.db'), => @setDatabasePhase('setup') @windowManager.showMainWindow() diff --git a/src/flux/stores/database-store.coffee b/src/flux/stores/database-store.coffee index 80a4d7236..fcb43300c 100644 --- a/src/flux/stores/database-store.coffee +++ b/src/flux/stores/database-store.coffee @@ -196,7 +196,7 @@ class DatabaseStore extends NylasStore console.error("DatabaseStore: Query #{query}, #{JSON.stringify(values)} failed #{err.toString()}") else duration = Date.now() - start - metadata = {duration: duration, resultLength: result?.length} + metadata = {duration: duration, resultLength: results?.length} console.debug(DEBUG_TO_LOG, "DatabaseStore: END (#{duration}) #{query}", metadata) if query is COMMIT diff --git a/src/flux/stores/message-body-processor.coffee b/src/flux/stores/message-body-processor.coffee new file mode 100644 index 000000000..615f687c0 --- /dev/null +++ b/src/flux/stores/message-body-processor.coffee @@ -0,0 +1,60 @@ +MessageUtils = require '../models/message-utils' +MessageStore = require './message-store' + +MessageBodyWidth = 740 + +class MessageBodyProcessor + + constructor: -> + @resetCache() + + resetCache: -> + # Store an object for recently processed items. Put the item reference into + # both data structures so we can access it in O(1) and also delete in O(1) + @_recentlyProcessedA = [] + @_recentlyProcessedD = {} + + process: (key, message) -> + body = message.body + if @_recentlyProcessedD[key] + return @_recentlyProcessedD[key].body + + # Give each extension the message object to process the body, but don't + # allow them to modify anything but the body for the time being. + for extension in MessageStore.extensions() + continue unless extension.formatMessageBody + virtual = message.clone() + virtual.body = body + extension.formatMessageBody(virtual) + body = virtual.body + + # Find inline images and give them a calculated CSS height based on + # html width and height, when available. This means nothing changes size + # as the image is loaded, and we can estimate final height correctly. + # Note that MessageBodyWidth must be updated if the UI is changed! + + while (result = MessageUtils.cidRegex.exec(body)) isnt null + imgstart = body.lastIndexOf('<', result.index) + imgend = body.indexOf('/>', result.index) + + if imgstart != -1 and imgend > imgstart + imgtag = body.substr(imgstart, imgend - imgstart) + width = imgtag.match(/width[ ]?=[ ]?['"]?(\d*)['"]?/)?[1] + height = imgtag.match(/height[ ]?=[ ]?['"]?(\d*)['"]?/)?[1] + if width and height + scale = Math.min(1, MessageBodyWidth / width) + style = " style=\"height:#{height * scale}px;\" " + body = body.substr(0, imgend) + style + body.substr(imgend) + + @addToCache(key, body) + body + + addToCache: (key, body) -> + if @_recentlyProcessedA.length > 50 + removed = @_recentlyProcessedA.pop() + delete @_recentlyProcessedD[removed.key] + item = {key, body} + @_recentlyProcessedA.unshift(item) + @_recentlyProcessedD[key] = item + +module.exports = new MessageBodyProcessor() diff --git a/src/flux/stores/message-store.coffee b/src/flux/stores/message-store.coffee index 3edc9a3b6..f79aa3bd2 100644 --- a/src/flux/stores/message-store.coffee +++ b/src/flux/stores/message-store.coffee @@ -53,6 +53,8 @@ class MessageStore extends NylasStore # registerExtension: (ext) => @_extensions.push(ext) + MessageBodyProcessor = require './message-body-processor' + MessageBodyProcessor.resetCache() # Public: Unregisters the extension provided from the MessageStore. # @@ -60,6 +62,8 @@ class MessageStore extends NylasStore # unregisterExtension: (ext) => @_extensions = _.without(@_extensions, ext) + MessageBodyProcessor = require './message-body-processor' + MessageBodyProcessor.resetCache() ########### PRIVATE #################################################### @@ -84,18 +88,23 @@ class MessageStore extends NylasStore if change.objectClass is Message.name inDisplayedThread = _.some change.objects, (obj) => obj.threadId is @_thread.id if inDisplayedThread - @_fetchFromCache() # Are we most likely adding a new draft? If the item is a draft and we don't # have it's local Id, optimistically add it to the set, resort, and trigger. # Note: this can avoid 100msec+ of delay from "Reply" => composer onscreen, item = change.objects[0] - if change.objects.length is 1 and item.draft is true and @_itemsLocalIds[item.id] is null + itemAlreadyExists = _.some @_items, (msg) -> msg.id is item.id + if change.objects.length is 1 and item.draft is true and not itemAlreadyExists DatabaseStore.localIdForModel(item).then (localId) => @_itemsLocalIds[item.id] = localId - @_items.push(item) + # We need to create a new copy of the items array so that the message-list + # can compare new state to previous state. + @_items = [].concat(@_items, [item]) @_items = @_sortItemsForDisplay(@_items) + @_expandItemsToDefault() @trigger() + else + @_fetchFromCache() if change.objectClass is Thread.name updatedThread = _.find change.objects, (obj) => obj.id is @_thread.id diff --git a/src/flux/tasks/destroy-draft.coffee b/src/flux/tasks/destroy-draft.coffee index 2f3e569be..9a9a7aed5 100644 --- a/src/flux/tasks/destroy-draft.coffee +++ b/src/flux/tasks/destroy-draft.coffee @@ -35,7 +35,7 @@ class DestroyDraftTask extends Task # when we performed locally, or if the draft has never been synced to # the server (id is still self-assigned) return Promise.resolve(Task.Status.Finished) unless @draft - return Promise.resolve(Task.Status.Finished) unless @draft.isSaved() + return Promise.resolve(Task.Status.Finished) unless @draft.isSaved() and @draft.version NylasAPI.makeRequest path: "/n/#{@draft.namespaceId}/drafts/#{@draft.id}"