[Ur] Substantial memory leak in the JavaScript runtime

[Adam Chlipala]

Thanks for spotting this problem.  I've pushed a different fix that 
makes dynamic input elements create <script> tags, so that the original 
dyn() code finds them to garbage-collect.  Does this change also seem to 
fix the problem?

(I'd like to avoid using createNodeIterator(), which only became widely 
deployed about 5 years ago.  I think Ur/Web client-side reactive code 
today will still work on browsers from about the year 2000, and I'd like 
to keep it that way unless there's a very compelling reason to the 
contrary.)


On 08/12/2016 09:55 PM, Saulo Araujo wrote:
> Hi,
>
> I believe there is a substantial memory leak in the JavaScript runtime 
> of Ur/Web. To see it happening:
>
> 1) visit 
> http://timesheet-ur.sauloaraujo.com:8080/TimeSheet/application with 
> Google Chrome
> 2) open the developer tools by pressing f12
> 3) click in the "Profiles" tab
> 4) click in the "Take Heap Snapshot" radio button
> 5) click in the "Take Snapshot" button
> 6) type Detached in the "Class filter" text box (you will see that the 
> heap has about 7MB and Detached DOM trees are retaining 0% of the 
> memory - https://snag.gy/2nF9fz.jpg)
>
> 7) click 10 times in the foward icon (the one on the right of the 
> "Date" header in the time sheet application)
> 9) click in the "Take Snapshot" button
> 10) type Detached in the "Class filter" text box (you will see that 
> the heap has about 43MB and Detached DOM trees are retaining 16% of 
> the memory - https://snag.gy/Crc3Vg.jpg)
>
> 11) click 10 times in the foward icon (the one on the right of the 
> "Date" header in the time sheet application)
> 12) click in the "Take Snapshot" button
> 13) type Detached in the "Class filter" text box (you will see that 
> the heap has about 78MB and Detached DOM trees are retaining 17% of 
> the memory - https://snag.gy/P5BLhq.jpg)
>
> I have spent quite some time investigating this memory leak and I 
> believe the problem is in the function recreate inside the function dyn:
>
> function dyn(pnode, s) {
>     if (suspendScripts)
>         return;
>
>     var x = document.createElement("script");
>     x.dead = false;
>     x.signal = s;
>     x.sources = null;
>     x.closures = null;
>
>     var firstChild = null;
>
>     x.recreate = function(v) {
>         for (var ls = x.closures; ls; ls = ls.next)
>             freeClosure(ls.data);
>
>         var next;
>         for (var child = firstChild; child && child != x; child = next) {
>             next = child.nextSibling;
>
>             killScript(child);
>             if (child.getElementsByTagName) {
>                 var arr = child.getElementsByTagName("script");
>                 for (var i = 0; i < arr.length; ++i)
>                     killScript(arr[i]);
>             }
> ...
>
> Note that recreate only kills <script> nodes . Therefore, <input>, 
> <textarea> and <select> nodes created through <ctextbox>, <ctextarea> 
> and <cselect> will continue to be in the dyns lists of its sources. To 
> fix this memory leak, I propose changing the function dyn to
>
> function dyn(pnode, s) {
>     if (suspendScripts)
>         return;
>
>     var x = document.createElement("script");
>     x.dead = false;
>     x.signal = s;
>     x.sources = null;
>     x.closures = null;
>
>     var firstChild = null;
>
>     x.recreate = function(v) {
>         for (var ls = x.closures; ls; ls = ls.next)
>             freeClosure(ls.data);
>
>         var next;
>         for (var child = firstChild; child && child != x; child = next) {
>             next = child.nextSibling;
>
>             killDyns(child)
> ...
>
> Below is the function killDyns.
>
> function killDyns(node) {
>     var nodeIterator = document.createNodeIterator(node, 
> NodeFilter.SHOW_ELEMENT, function (node) {
>         return node.dead !== undefined && node.dead === false
>     })
>     var node = nodeIterator.nextNode();
>     while (node) {
>         killScript(node);
>         node = nodeIterator.nextNode();
>     }
> }
>
> killDyns traverses all descendants of a node that have a dead 
> attribute equal to false and kills them with killScript. Therefore, 
> killDyns may be less performant than the code it substitutes, but I 
> believe killDyns is less prone to memory leaks than the original code. 
> There is another small change to be done in urweb.js. You can see all 
> changes in 
> https://github.com/saulo2/urweb/commit/dcd280c85595ceee60a4fb78a3bfaf413a9eb7b8#diff-867e8dfbbc36419eefc0dfdf9db32883
>
> I did not make a pull request yet because I would like to know the 
> opinion of the Ur/Web comitters about this fix. Maybe there is 
> something that can be improved. Maybe there is a new bug in it :) In 
> any case, repeating the previous steps with the proposed changes, I 
> get the following results:
>
> 6) type Detached in the "Class filter" text box (you will see that the 
> heap has about 7MB and Detached DOM trees are retaining 0% of the 
> memory - https://snag.gy/NicJow.jpg)
> 10) type Detached in the "Class filter" text box (you will see that 
> the heap has about 15MB and Detached DOM trees are retaining 0% of the 
> memory - https://snag.gy/nuVfhg.jpg)
> 13) type Detached in the "Class filter" text box (you will see that 
> the heap has about 21MB and Detached DOM trees are retaining 0% of the 
> memory - https://snag.gy/aPeUuK.jpg)
>
> The new results suggest that there is another memory leak in the 
> JavaScript Runtime. However, I have not looked into it yet.
>
> Sincerely,
> Saulo
>
>
> _______________________________________________
> Ur mailing list
> Ur at impredicative.com
> http://www.impredicative.com/cgi-bin/mailman/listinfo/ur

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.impredicative.com/pipermail/ur/attachments/20160824/648ff280/attachment.html>