{"thread":{"id":"27482","subject":"[PATCH 3/3] gitweb.js: use setTimeout rather than setInterval in blame_incremental.js","startedAt":"2011-05-27T13:49:58Z","lastAt":"2011-05-27T14:04:38Z","messageCount":5,"participants":["Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"168869","messageId":"1306504201-18014-1-git-send-email-jnareb@gmail.com","threadId":"27482","inReplyTo":null,"subject":"[PATCH 0/3] gitweb: Improving blame_incremental.js","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-05-27T13:49:58Z","receivedAt":"2011-05-27T13:49:58Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Two first patches remove unnecessary code from JavaScript-side of\nblame_incremental code.  Namely JavaScript is single-threaded (events\nwait for interpreter), so there is no need for code that tried to\nhandle re-entrancy (concurent access) to blame_incremental.js\nfunctions.\n\nSo those two are pure simplification.\n\n\nThe third (last) patch replaces setInterval (where events might\naccumulate if browser is very busy) to recommended re-enabled\nsetTimeout.  Thanks to this change we are now able to detect if timer\nis not necessary, and in that case disable it.  This means extra code\nchecking if there is timer to disable (perhaps unnecessary).\n\nThis one adds more code than it removes, and could be split into two\npatches: one simply moving from setInterval to setTimer, second adding\nthose new features.\n\n\nP.S. Does anybody knows how to test JavaScript part of gitweb code\n     _from commandline_ generating TAP-compatibile output to stdout?\n\n\nShortlog:\n~~~~~~~~~\nJakub Narebski (3):\n  gitweb.js: No need for inProgress in blame_incremental.js\n  gitweb.js: No need for loop in blame_incremental's handleResponse()\n  gitweb.js: use setTimeout rather than setInterval in\n    blame_incremental.js\n\nDiffstat:\n~~~~~~~~~\n gitweb/static/js/blame_incremental.js |   79 +++++++++++++++++---------------\n 1 files changed, 42 insertions(+), 37 deletions(-)\n\n-- \n1.7.5\n"},{"id":"168870","messageId":"1306504201-18014-2-git-send-email-jnareb@gmail.com","threadId":"27482","inReplyTo":"1306504201-18014-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 1/3] gitweb.js: No need for inProgress in blame_incremental.js","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-05-27T13:49:59Z","receivedAt":"2011-05-27T13:49:59Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"JavaScript is single-threaded, so there is no need for protection\nagainst re-entrancy via inProgress variable.\n\nIn particular calls to setInterval handler are stacked if handler\ndoesn't finish before new interrupt (before new interval).  The same\nhappens with events - they are (hopefully) stacked if even handler\ndidn't finish work.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nRemove and simplify code.\n\n gitweb/static/js/blame_incremental.js |   21 +++------------------\n 1 files changed, 3 insertions(+), 18 deletions(-)\n\ndiff --git a/gitweb/static/js/blame_incremental.js b/gitweb/static/js/blame_incremental.js\nindex 676da6b..4841805 100644\n--- a/gitweb/static/js/blame_incremental.js\n+++ b/gitweb/static/js/blame_incremental.js\n@@ -420,8 +420,6 @@ function handleLine(commit, group) {\n \n // ----------------------------------------------------------------------\n \n-var inProgress = false;   // are we processing response\n-\n /**#@+\n  * @constant\n  */\n@@ -536,7 +534,7 @@ function processData(unprocessed, nextReadPos) {\n  *\n  * @param {XMLHttpRequest} xhr: XMLHttpRequest object\n  *\n- * @globals pollTimer, commits, inProgress\n+ * @globals pollTimer, commits\n  */\n function handleError(xhr) {\n \terrorInfo('Server error: ' +\n@@ -544,8 +542,6 @@ function handleError(xhr) {\n \n \tclearInterval(pollTimer);\n \tcommits = {}; // free memory\n-\n-\tinProgress = false;\n }\n \n /**\n@@ -553,7 +549,7 @@ function handleError(xhr) {\n  *\n  * @param {XMLHttpRequest} xhr: XMLHttpRequest object (unused)\n  *\n- * @globals pollTimer, commits, inProgress\n+ * @globals pollTimer, commits\n  */\n function responseLoaded(xhr) {\n \tclearInterval(pollTimer);\n@@ -561,15 +557,13 @@ function responseLoaded(xhr) {\n \tfixColorsAndGroups();\n \twriteTimeInterval();\n \tcommits = {}; // free memory\n-\n-\tinProgress = false;\n }\n \n /**\n  * handler for XMLHttpRequest onreadystatechange event\n  * @see startBlame\n  *\n- * @globals xhr, inProgress\n+ * @globals xhr\n  */\n function handleResponse() {\n \n@@ -609,13 +603,6 @@ function handleResponse() {\n \t\treturn;\n \t}\n \n-\t// in case we were called before finished processing\n-\tif (inProgress) {\n-\t\treturn;\n-\t} else {\n-\t\tinProgress = true;\n-\t}\n-\n \t// extract new whole (complete) lines, and process them\n \twhile (xhr.prevDataLength !== xhr.responseText.length) {\n \t\tif (xhr.readyState === 4 &&\n@@ -633,8 +620,6 @@ function handleResponse() {\n \t    xhr.prevDataLength === xhr.responseText.length) {\n \t\tresponseLoaded(xhr);\n \t}\n-\n-\tinProgress = false;\n }\n \n // ============================================================\n-- \n1.7.5\n"},{"id":"168871","messageId":"1306504201-18014-3-git-send-email-jnareb@gmail.com","threadId":"27482","inReplyTo":"1306504201-18014-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 2/3] gitweb.js: No need for loop in blame_incremental's handleResponse()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-05-27T13:50:00Z","receivedAt":"2011-05-27T13:50:00Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"JavaScript is single-threaded, so there is no need for protecting\nagainst changes to XMLHttpRequest object behind event handler back.\n\nTherefore there is no need for loop that was here in case `xhr' got\nnew changes while processing current changes.  This should make code a\nbit more clear.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nRemove and simplify code.\n\n gitweb/static/js/blame_incremental.js |   13 ++++---------\n 1 files changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/gitweb/static/js/blame_incremental.js b/gitweb/static/js/blame_incremental.js\nindex 4841805..27955ec 100644\n--- a/gitweb/static/js/blame_incremental.js\n+++ b/gitweb/static/js/blame_incremental.js\n@@ -603,21 +603,16 @@ function handleResponse() {\n \t\treturn;\n \t}\n \n-\t// extract new whole (complete) lines, and process them\n-\twhile (xhr.prevDataLength !== xhr.responseText.length) {\n-\t\tif (xhr.readyState === 4 &&\n-\t\t    xhr.prevDataLength === xhr.responseText.length) {\n-\t\t\tbreak;\n-\t\t}\n \n+\t// extract new whole (complete) lines, and process them\n+\tif (xhr.prevDataLength !== xhr.responseText.length) {\n \t\txhr.prevDataLength = xhr.responseText.length;\n \t\tvar unprocessed = xhr.responseText.substring(xhr.nextReadPos);\n \t\txhr.nextReadPos = processData(unprocessed, xhr.nextReadPos);\n-\t} // end while\n+\t}\n \n \t// did we finish work?\n-\tif (xhr.readyState === 4 &&\n-\t    xhr.prevDataLength === xhr.responseText.length) {\n+\tif (xhr.readyState === 4) {\n \t\tresponseLoaded(xhr);\n \t}\n }\n-- \n1.7.5\n"},{"id":"168868","messageId":"1306504201-18014-4-git-send-email-jnareb@gmail.com","threadId":"27482","inReplyTo":"1306504201-18014-1-git-send-email-jnareb@gmail.com","subject":"[PATCH 3/3] gitweb.js: use setTimeout rather than setInterval in blame_incremental.js","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-05-27T13:50:01Z","receivedAt":"2011-05-27T13:50:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"If there is a possibility that your logic could take longer to execute\nthan the interval time, it is recommended that you recursively call a\nnamed function using window.setTimeout rather than window.setInterval.\n\nTherefore instead of using setInterval as an alternate way of invoking\nhandleResponse (because some web browsers call onreadystatechange only\nonce per each distinct state, and not for each server flush), use\nsetTimeout and reset it from handleResponse.  As a bonus this allows\nus to get rid of timer if it turns out that web browser calls\nonreadystatechange on each server flush.\n\nWhile at it get rid of `xhr' global variable, creating it instead as\nlocal variable in startBlame and passing it as parameter, and of\n`pollTimer' global variable, passing it as member of xhr object\n(xhr.pollTimer).\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis patch introduces three separate features, and probably should be\nsplit into separate patches.\n\n gitweb/static/js/blame_incremental.js |   55 ++++++++++++++++++++++++---------\n 1 files changed, 40 insertions(+), 15 deletions(-)\n\ndiff --git a/gitweb/static/js/blame_incremental.js b/gitweb/static/js/blame_incremental.js\nindex 27955ec..91354a0 100644\n--- a/gitweb/static/js/blame_incremental.js\n+++ b/gitweb/static/js/blame_incremental.js\n@@ -29,7 +29,6 @@\n /* ............................................................ */\n /* utility/helper functions (and variables) */\n \n-var xhr;        // XMLHttpRequest object\n var projectUrl; // partial query + separator ('?' or ';')\n \n // 'commits' is an associative map. It maps SHA1s to Commit objects.\n@@ -431,8 +430,6 @@ var endRe  = /^END ?([^ ]*) ?(.*)/;\n var curCommit = new Commit();\n var curGroup  = {};\n \n-var pollTimer = null;\n-\n /**\n  * Parse output from 'git blame --incremental [...]', received via\n  * XMLHttpRequest from server (blamedataUrl), and call handleLine\n@@ -533,26 +530,34 @@ function processData(unprocessed, nextReadPos) {\n  * Handle XMLHttpRequest errors\n  *\n  * @param {XMLHttpRequest} xhr: XMLHttpRequest object\n+ * @param {Number} [xhr.pollTimer] ID of the timeout to clear\n  *\n- * @globals pollTimer, commits\n+ * @globals commits\n  */\n function handleError(xhr) {\n \terrorInfo('Server error: ' +\n \t\txhr.status + ' - ' + (xhr.statusText || 'Error contacting server'));\n \n-\tclearInterval(pollTimer);\n+\tif (typeof xhr.pollTimer === \"number\") {\n+\t\tclearTimeout(xhr.pollTimer);\n+\t\tdelete xhr.pollTimer;\n+\t}\n \tcommits = {}; // free memory\n }\n \n /**\n  * Called after XMLHttpRequest finishes (loads)\n  *\n- * @param {XMLHttpRequest} xhr: XMLHttpRequest object (unused)\n+ * @param {XMLHttpRequest} xhr: XMLHttpRequest object\n+ * @param {Number} [xhr.pollTimer] ID of the timeout to clear\n  *\n- * @globals pollTimer, commits\n+ * @globals commits\n  */\n function responseLoaded(xhr) {\n-\tclearInterval(pollTimer);\n+\tif (typeof xhr.pollTimer === \"number\") {\n+\t\tclearTimeout(xhr.pollTimer);\n+\t\tdelete xhr.pollTimer;\n+\t}\n \n \tfixColorsAndGroups();\n \twriteTimeInterval();\n@@ -563,9 +568,13 @@ function responseLoaded(xhr) {\n  * handler for XMLHttpRequest onreadystatechange event\n  * @see startBlame\n  *\n- * @globals xhr\n+ * @param {XMLHttpRequest} xhr: XMLHttpRequest object\n+ * @param {Number} xhr.prevDataLength: previous value of xhr.responseText.length\n+ * @param {Number} xhr.nextReadPos: start of unread part of xhr.responseText\n+ * @param {Number} [xhr.pollTimer] ID of the timeout (to reset or cancel)\n+ * @param {Boolean} fromTimer: if handler was called from timer\n  */\n-function handleResponse() {\n+function handleResponse(xhr, fromTimer) {\n \n \t/*\n \t * xhr.readyState\n@@ -614,6 +623,19 @@ function handleResponse() {\n \t// did we finish work?\n \tif (xhr.readyState === 4) {\n \t\tresponseLoaded(xhr);\n+\t\treturn;\n+\t}\n+\n+\t// if we get from timer, we have to restart it\n+\t// otherwise onreadystatechange gives us partial response, timer not needed\n+\tif (fromTimer) {\n+\t\tsetTimeout(function () {\n+\t\t\thandleResponse(xhr, true);\n+\t\t}, 1000);\n+\n+\t} else if (typeof xhr.pollTimer === \"number\") {\n+\t\tclearTimeout(xhr.pollTimer);\n+\t\tdelete xhr.pollTimer;\n \t}\n }\n \n@@ -629,11 +651,11 @@ function handleResponse() {\n  * Called from 'blame_incremental' view after loading table with\n  * file contents, a base for blame view.\n  *\n- * @globals xhr, t0, projectUrl, div_progress_bar, totalLines, pollTimer\n+ * @globals t0, projectUrl, div_progress_bar, totalLines\n */\n function startBlame(blamedataUrl, bUrl) {\n \n-\txhr = createRequestObject();\n+\tvar xhr = createRequestObject();\n \tif (!xhr) {\n \t\terrorInfo('ERROR: XMLHttpRequest not supported');\n \t\treturn;\n@@ -652,8 +674,9 @@ function startBlame(blamedataUrl, bUrl) {\n \txhr.prevDataLength = -1;  // used to detect if we have new data\n \txhr.nextReadPos = 0;      // where unread part of response starts\n \n-\txhr.onreadystatechange = handleResponse;\n-\t//xhr.onreadystatechange = function () { handleResponse(xhr); };\n+\txhr.onreadystatechange = function () {\n+\t\thandleResponse(xhr, false); \n+\t};\n \n \txhr.open('GET', blamedataUrl);\n \txhr.setRequestHeader('Accept', 'text/plain');\n@@ -661,7 +684,9 @@ function startBlame(blamedataUrl, bUrl) {\n \n \t// not all browsers call onreadystatechange event on each server flush\n \t// poll response using timer every second to handle this issue\n-\tpollTimer = setInterval(xhr.onreadystatechange, 1000);\n+\txhr.pollTimer = setTimeout(function () {\n+\t\thandleResponse(xhr, true);\n+\t}, 1000);\n }\n \n /* end of blame_incremental.js */\n-- \n1.7.5\n"},{"id":"168875","messageId":"201105271604.41731.jnareb@gmail.com","threadId":"27482","inReplyTo":"1306504201-18014-1-git-send-email-jnareb@gmail.com","subject":"Re: [PATCH 0/3] gitweb: Improving blame_incremental.js","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-05-27T14:04:38Z","receivedAt":"2011-05-27T14:04:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jakub Narebski wrote:\n\n> Two first patches remove unnecessary code from JavaScript-side of\n> blame_incremental code. [...]\n> \n> So those two are pure simplification.\n> \n> \n> The third (last) patch replaces setInterval (where events might\n> accumulate if browser is very busy) to recommended re-enabled\n> setTimeout.  [...]\n> \n> This one adds more code than it removes, and could be split into two\n> patches: [...]\n\nI forgot to add that this series is based on top of jn/gitweb-js\nseries, namely it theoretically require \"gitweb: Split JavaScript for\nmaintability, combining on build\"... but thanks to Git rename detection\nit should [had] apply on top of master as well.\n\n-- \nJakub Narebski\nPoland\n"}]}