Patchwork [2,of,2] hgweb: optimize process_dates function

login
register
mail settings
Submitter Alexander Plavin
Date Aug. 8, 2013, 1:16 p.m.
Message ID <2e3064b1a8b3719bc896.1375967817@debian-alexander.dolgopa>
Download mbox | patch
Permalink /patch/2096/
State Superseded
Headers show

Comments

Alexander Plavin - Aug. 8, 2013, 1:16 p.m.
# HG changeset patch
# User Alexander Plavin <alexander@plav.in>
# Date 1374682432 -14400
#      Wed Jul 24 20:13:52 2013 +0400
# Node ID 2e3064b1a8b3719bc896e61a5804fd515a0bc9cb
# Parent  d2e91a2d576deff3bb97fefc1fe71c2a64d64bb6
hgweb: optimize process_dates function

This function looped over every node due to getElementsByTagName('*'), instead
of using selectors. In this patch we use querySelectorAll('.age') and process
only these nodes, which is much faster and also doesn't require extra condition.

Browser compatibility isn't sacrificed: IE 8+, FF 3.5+, Opera 10+.
Laurens Holst - Aug. 10, 2013, 12:56 p.m.
Op 08-08-13 15:16, Alexander Plavin schreef:
> # HG changeset patch
> # User Alexander Plavin <alexander@plav.in>
> # Date 1374682432 -14400
> #      Wed Jul 24 20:13:52 2013 +0400
> # Node ID 2e3064b1a8b3719bc896e61a5804fd515a0bc9cb
> # Parent  d2e91a2d576deff3bb97fefc1fe71c2a64d64bb6
> hgweb: optimize process_dates function
>
> This function looped over every node due to getElementsByTagName('*'), instead
> of using selectors. In this patch we use querySelectorAll('.age') and process
> only these nodes, which is much faster and also doesn't require extra condition.
>
> Browser compatibility isn't sacrificed: IE 8+, FF 3.5+, Opera 10+.

How much do we care about compatibility with IE8?

> diff -r d2e91a2d576d -r 2e3064b1a8b3 mercurial/templates/static/mercurial.js
> --- a/mercurial/templates/static/mercurial.js	Wed Jul 24 18:53:55 2013 +0400
> +++ b/mercurial/templates/static/mercurial.js	Wed Jul 24 20:13:52 2013 +0400
> @@ -245,21 +245,18 @@
>   		}
>   	}
>   
> -    var nodes = document.getElementsByTagName('*');
> -    var ageclass = new RegExp('\\bage\\b');
> +    var nodes = document.querySelectorAll('.age');

Possibly getElementsByClassName() is more efficient.

Looks like it’s only supported in IE9 though. (Other browsers actually 
had it *before* querySelectorAll().)

>       var dateclass = new RegExp('\\bdate\\b');

I would use a literal /\bdate\b/ instead of new RegExp().

>       for (var i=0; i<nodes.length; ++i){
>           var node = nodes[i];
>           var classes = node.className;
> -        if (ageclass.test(classes)){
> -            var agevalue = age(node.textContent);
> -            if (dateclass.test(classes)){
> -                // We want both: date + (age)
> -                node.textContent += ' ('+agevalue+')';
> -            } else {
> -                node.title = node.textContent;
> -                node.textContent = agevalue;
> -            }
> +        var agevalue = age(node.textContent);
> +        if (dateclass.test(classes)){

As you’re using classList elsewhere, I think you could use 
classList.contains() instead of the regexp match?

Though MDN tells me it is only supported in IE10 onwards, but if that’s 
an argument then we shouldn’t be using it in the other place either.

> +            // We want both: date + (age)
> +            node.textContent += ' ('+agevalue+')';
> +        } else {
> +            node.title = node.textContent;
> +            node.textContent = agevalue;
>           }
>       }
>   }
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>
Alexander Plavin - Aug. 12, 2013, 5:36 p.m.
10.08.2013, 16:57, "Laurens Holst" <laurens.nospam@grauw.nl>:
> Op 08-08-13 15:16, Alexander Plavin schreef:
>
>>  # HG changeset patch
>>  # User Alexander Plavin <alexander@plav.in>
>>  # Date 1374682432 -14400
>>  #      Wed Jul 24 20:13:52 2013 +0400
>>  # Node ID 2e3064b1a8b3719bc896e61a5804fd515a0bc9cb
>>  # Parent  d2e91a2d576deff3bb97fefc1fe71c2a64d64bb6
>>  hgweb: optimize process_dates function
>>
>>  This function looped over every node due to getElementsByTagName('*'), instead
>>  of using selectors. In this patch we use querySelectorAll('.age') and process
>>  only these nodes, which is much faster and also doesn't require extra condition.
>>
>>  Browser compatibility isn't sacrificed: IE 8+, FF 3.5+, Opera 10+.
>
> How much do we care about compatibility with IE8?

May be not much, but it's not bad anyway :)

>
>>  diff -r d2e91a2d576d -r 2e3064b1a8b3 mercurial/templates/static/mercurial.js
>>  --- a/mercurial/templates/static/mercurial.js Wed Jul 24 18:53:55 2013 +0400
>>  +++ b/mercurial/templates/static/mercurial.js Wed Jul 24 20:13:52 2013 +0400
>>  @@ -245,21 +245,18 @@
>>                    }
>>            }
>>
>>  -    var nodes = document.getElementsByTagName('*');
>>  -    var ageclass = new RegExp('\\bage\\b');
>>  +    var nodes = document.querySelectorAll('.age');
>
> Possibly getElementsByClassName() is more efficient.

Yes, it's more efficient of course, but:
- querying by a selector gives a very easy way to add some scope limit, see http://mercurial.markmail.org/message/kwcml3c6ewwonssr?q=process_dates
- this part (I mean getting list of nodes) is anyway much faster than actual processing of the nodes below

>
> Looks like it’s only supported in IE9 though. (Other browsers actually
> had it *before* querySelectorAll().)

As for me, querySelectorAll is supported for so long that we definitely shouldn't care about this, and this shouldn't be an argument against it.

>
>>        var dateclass = new RegExp('\\bdate\\b');
>
> I would use a literal /\bdate\b/ instead of new RegExp().

I would too, but it's not that good to add (I think) to add this change to this patch.

>
>>        for (var i=0; i<nodes.length; ++i){
>>            var node = nodes[i];
>>            var classes = node.className;
>>  -        if (ageclass.test(classes)){
>>  -            var agevalue = age(node.textContent);
>>  -            if (dateclass.test(classes)){
>>  -                // We want both: date + (age)
>>  -                node.textContent += ' ('+agevalue+')';
>>  -            } else {
>>  -                node.title = node.textContent;
>>  -                node.textContent = agevalue;
>>  -            }
>>  +        var agevalue = age(node.textContent);
>>  +        if (dateclass.test(classes)){
>
> As you’re using classList elsewhere, I think you could use
> classList.contains() instead of the regexp match?

It's already used (by me :) ) in line wrapping switch, and in my opinion getting readable dates (this place, process_dates) is more important than having the possibility to switch line wrapping dynamically. Also, this part of code existed before this patch, so why change it, what's the reason?

>
> Though MDN tells me it is only supported in IE10 onwards, but if that’s
> an argument then we shouldn’t be using it in the other place either.
>
>>  +            // We want both: date + (age)
>>  +            node.textContent += ' ('+agevalue+')';
>>  +        } else {
>>  +            node.title = node.textContent;
>>  +            node.textContent = agevalue;
>>            }
>>        }
>>    }
>>  _______________________________________________
>>  Mercurial-devel mailing list
>>  Mercurial-devel@selenic.com
>>  http://selenic.com/mailman/listinfo/mercurial-devel
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Aug. 19, 2013, 7:11 p.m.
On Sat, 2013-08-10 at 14:56 +0200, Laurens Holst wrote:
> Op 08-08-13 15:16, Alexander Plavin schreef:
> > # HG changeset patch
> > # User Alexander Plavin <alexander@plav.in>
> > # Date 1374682432 -14400
> > #      Wed Jul 24 20:13:52 2013 +0400
> > # Node ID 2e3064b1a8b3719bc896e61a5804fd515a0bc9cb
> > # Parent  d2e91a2d576deff3bb97fefc1fe71c2a64d64bb6
> > hgweb: optimize process_dates function
> >
> > This function looped over every node due to getElementsByTagName('*'), instead
> > of using selectors. In this patch we use querySelectorAll('.age') and process
> > only these nodes, which is much faster and also doesn't require extra condition.
> >
> > Browser compatibility isn't sacrificed: IE 8+, FF 3.5+, Opera 10+.
> 
> How much do we care about compatibility with IE8?

A lot. Here's a site that suggests it's the most popular version of IE
still:

http://www.w3schools.com/browsers/browsers_explorer.asp

Patch

diff -r d2e91a2d576d -r 2e3064b1a8b3 mercurial/templates/static/mercurial.js
--- a/mercurial/templates/static/mercurial.js	Wed Jul 24 18:53:55 2013 +0400
+++ b/mercurial/templates/static/mercurial.js	Wed Jul 24 20:13:52 2013 +0400
@@ -245,21 +245,18 @@ 
 		}
 	}
 
-    var nodes = document.getElementsByTagName('*');
-    var ageclass = new RegExp('\\bage\\b');
+    var nodes = document.querySelectorAll('.age');
     var dateclass = new RegExp('\\bdate\\b');
     for (var i=0; i<nodes.length; ++i){
         var node = nodes[i];
         var classes = node.className;
-        if (ageclass.test(classes)){
-            var agevalue = age(node.textContent);
-            if (dateclass.test(classes)){
-                // We want both: date + (age)
-                node.textContent += ' ('+agevalue+')';
-            } else {
-                node.title = node.textContent;
-                node.textContent = agevalue;
-            }
+        var agevalue = age(node.textContent);
+        if (dateclass.test(classes)){
+            // We want both: date + (age)
+            node.textContent += ' ('+agevalue+')';
+        } else {
+            node.title = node.textContent;
+            node.textContent = agevalue;
         }
     }
 }