-
Notifications
You must be signed in to change notification settings - Fork 195
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
Performance issues w 200 annotations on a page #556
Comments
Original Slack discussion for context: https://hypothes-is.slack.com/archives/C03QZM0K4/p1507829093000092 |
The page has >800 annotations at the time of writing. Once the annotations are actually loaded the sidebar and page is responsive enough to be usable. The sidebar is very laggy while annotations are being fetched and anchored though, a process that takes 30 seconds+ on my 2017 MBP. From a quick Chrome profile, it looks like a significant amount of time is being spent handling the |
I confirmed that the patch below which batches together processing of anchoring status updates in the sidebar significantly improves the situation. The remaining work is to update the tests, test it out on a bunch of real world web pages and in particular cases where there are no orphans, a few orphans and many orphans respectively. diff --git a/src/sidebar/frame-sync.js b/src/sidebar/frame-sync.js
index 47e74d46..8184ce51 100644
--- a/src/sidebar/frame-sync.js
+++ b/src/sidebar/frame-sync.js
@@ -1,5 +1,7 @@
'use strict';
+var debounce = require('lodash.debounce');
+
var events = require('./events');
var bridgeEvents = require('../shared/bridge-events');
var metadata = require('./annotation-metadata');
@@ -126,12 +128,20 @@ function FrameSync($rootScope, $window, Discovery, annotationUI, bridge) {
bridge.on('destroyFrame', destroyFrame.bind(this));
+ // Map of annotation tag to orphan status.
+ var orphanStateUpdates = {};
+ var debouncedSync = debounce(() => {
+ annotationUI.updateAnchorStatus(orphanStateUpdates);
+ $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, Object.keys(orphanStateUpdates));
+ orphanStateUpdates = {};
+ }, 10);
+
// Anchoring an annotation in the frame completed
bridge.on('sync', function (events_) {
events_.forEach(function (event) {
inFrame.add(event.tag);
- annotationUI.updateAnchorStatus(null, event.tag, event.msg.$orphan);
- $rootScope.$broadcast(events.ANNOTATIONS_SYNCED, [event.tag]);
+ orphanStateUpdates[event.tag] = event.msg.$orphan;
+ debouncedSync();
});
});
diff --git a/src/sidebar/reducers/annotations.js b/src/sidebar/reducers/annotations.js
index 5a0c2c39..f374759d 100644
--- a/src/sidebar/reducers/annotations.js
+++ b/src/sidebar/reducers/annotations.js
@@ -173,17 +173,13 @@ var update = {
UPDATE_ANCHOR_STATUS: function (state, action) {
var annotations = state.annotations.map(function (annot) {
- var match = (annot.id && annot.id === action.id) ||
- (annot.$tag && annot.$tag === action.tag);
- if (match) {
- return Object.assign({}, annot, {
- $anchorTimeout: action.anchorTimeout || annot.$anchorTimeout,
- $orphan: action.isOrphan,
- $tag: action.tag,
- });
- } else {
+ if (!action.orphanStates.hasOwnProperty(annot.$tag)) {
return annot;
}
+ return Object.assign({}, annot, {
+ $anchorTimeout: annot.$anchorTimeout,
+ $orphan: action.orphanStates[annot.$tag],
+ });
});
return {annotations: annotations};
},
@@ -299,17 +295,12 @@ function clearAnnotations() {
/**
* Updating the local tag and anchoring status of an annotation.
*
- * @param {string|null} id - Annotation ID
- * @param {string} tag - The local tag assigned to this annotation to link
- * the object in the page and the annotation in the sidebar
- * @param {boolean} isOrphan - True if the annotation failed to anchor
+ * @param {Object} orphanStates - A map of annotation tag to orphan status
*/
-function updateAnchorStatus(id, tag, isOrphan) {
+function updateAnchorStatus(orphanStates) {
return {
type: actions.UPDATE_ANCHOR_STATUS,
- id: id,
- tag: tag,
- isOrphan: isOrphan,
+ orphanStates,
};
}
|
Excellent! I had thought (https://hypothes-is.slack.com/archives/C07J6DNUB/p1502492449443310) this might be a messaging bottleneck, and had hoped batching calls across the bridge would help. Glad to see that it will. |
Let us know if we can be helpful testing on pages. I think we've got a number of them that are good examples from days gone by. Oddly, one of those pages might be http://web.hypothes.is |
Profiling the test case in #556 showed significant overhead from the processing involved in or triggered by each `UPDATE_ANCHOR_STATUS` action handled by the store. Previously one `UPDATE_ANCHOR_STATUS` action was dispatched for each annotation whose anchoring status changed. Improve the situation by making the `UPDATE_ANCHOR_STATUS` action handle updates for multiple annotations at once and modifying the `frameSync` service to coalesce anchoring status updates from the host page into a smaller number of `UPDATE_ANCHOR_STATUS` actions. This commit also takes the opportunity to remove the logic to update annotation tags when handling this action since it is no longer required. Fixes #556
Also see: #189 |
Profiling the test case in #556 showed significant overhead from the processing involved in or triggered by each `UPDATE_ANCHOR_STATUS` action handled by the store. Previously one `UPDATE_ANCHOR_STATUS` action was dispatched for each annotation whose anchoring status changed. Improve the situation by making the `UPDATE_ANCHOR_STATUS` action handle updates for multiple annotations at once and modifying the `frameSync` service to coalesce anchoring status updates from the host page into a smaller number of `UPDATE_ANCHOR_STATUS` actions. Fixes #556
Steps to reproduce
Expected behaviour
Page and sidebar performance should be acceptable
Actual behaviour
Quite slow, close to unusable.
Browser/system information
Chrome
The text was updated successfully, but these errors were encountered: