514639 - Serve code (mainly C++) files "as is" and use javascript on the client to format them

Status: RESOLVED FIXED

(Webtools Graveyard :: MXR, enhancement)

Description

[Jordan Osete]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: 

Currently when you are viewing some cpp file, MXR serves an html file with syntax highlighting already done, line numbers (<a class='l d1' name=49 href="#49">49</a>), and links to idents (<a class="d" href="/mozilla1.9.2/ident?i=openAbused">openAbused</a>), all already in place.

However, the file served is then about four or five times the size of the initial file. While this may not seem like a problem for small files, big ones get needlessly slow to load (example: http://mxr.mozilla.org/mozilla1.9.2/source/js/src/jstracer.cpp is 2.3 MB, while the original file is only 407 kB).

Also, most of the additional information (if not all) can be simply added on the client side, via JavaScript (with regular expressions or the like).
For example, the server could serve the file inside a div / monospace / white-space: pre, and the JS would then onload do the replacements that need to be done.

This would also reduce greatly the load on the mxr server (don't know if it is an issue).

Reproducible: Always

Steps to Reproduce:
1. view any C++ file through MXR, like this one (78kB): http://mxr.mozilla.org/mozilla1.9.2/source/dom/base/nsPIDOMWindow.h
Actual Results:  
The file is 78kB, four times more than the original (17kB) file.

Expected Results:  
The file should be way smaller, about the same size as the original file (plus the html header and JS file inclusion)

Comment 1

[timeless]

so, you're free to use dxr, add ?raw=1 to your url, or ...

i personally use mxr with js disabled for most of the net, including mxr.

If you provided a patch to enable this which relied on a cookie (and used script to set the cookie to enable the feature), i'd probably integrate it...
http://hg.mozilla.org/webtools/mxr

is the repo, and 'source' is the main script.

i don't think the mxr server is particularly annoyed by the load. users otoh are definitely annoyed by the load times.

Comment 2

[Jordan Osete]

I can do the JS part, however I think I will have some trouble with the perl part, as I don't "speak" perl...
I'll try to look into it.

Comment 3

[timeless]

?raw=1 exists, so you can look at how that works...

Comment 4

[Jordan Osete]

Yeah, thanks for the pointer.
My current guess is I could try to adapt markupfile (lib/LXR/common.pm) so that it outputs anything the JS will need to do its own syntax coloration (ie. the terms for the given language in a JS array, the full paths of the include files used...) and then print the raw code. Onload the JS will use the data to color stuff.

The current behaviour will be kept if JS is disabled.
However I am a lil' bit busy right now, but I hope I'll have more time to look into it next week.

Comment 5

[Jordan Osete]

Attachment: Partial patch, only lines number are added via JS, ~20% space gain only

I'm slower than expected, sorry.
This is a partial patch, it only moves the displaying of lines numbers from server-side perl to JS.
This reduces the size of the generated HTML by only 20% (roughly, depending on the file). http://mxr.mozilla.org/mozilla1.9.2/source/js/src/jstracer.cpp is now "only" 1.9 MB instead of 2.3
I'll try and improve it as I have time.

Comment 6

[Jordan Osete]

Attachment: Final patch, links to identifiers and some common stuff compressed, ~70% total space gain. Includes basic test page.

(This patch builds on the previous patch, so you need to apply both.)

It turns out there was no need to recreate or recode the parsing / coloration logic in JS, after all. This avoids duplicating the logic and keeps the patch simple.
The coloration / formating is still made on the server side by perl, then if JS is enabled, perl "compresses" some of the overhead generated for the most common formatting, and JS decompresses it.

Mainly:
- let the JS add the line-numbers instead of the perl script (removing thousands of "<a class='l d1' name=32 href="#32">32</a>"). This is done by the first patch.
- end tags are compressed to a single greater than sign ">". This suppresses a common HTML/XML redundancy (but requires valid markup).
- '<span class="v">' is replaced by '<V'. By using a capital single letter, I avoid possible collision with existing tags (which are all lowercase). A few spans with single-letter classes that are quite common in the HTML that the perl script generates are compressed this way (letters are ACVSI).
- links to identifiers in the form '<a class="d" href="/mxr/mxr-JS2/zetests/ident?i=Cc">Cc</a>' which are  extremely redundant, are replaced by <DCc> ("<D" followed by the identifier, then a greater-than sign marking the end of the tag). This in itself provides the biggest gain, usually more than 30%, depending on the file.
- clickable links and mailtos that are wrapped in <a...>...</a> are also reduced to <L...> and <M...>

All in all, the gain provided by this patch is often more than 70% on most cpp files I tried it on. The file I previously took as example, jstracer.cpp, gets a whooping 78% boost !
The method used (replacing common stuff with a single-capital-letter start tag <V <D, etc. to reduce overhead) also seems quite foolproof to me, and is easily extendable as well, with all those 26 letters people have chosen to put in our alphabet ;)

I also include a basic test page in the patch (in tests/check_jscoloration.html). This uses jQuery, and eases checking that the markup generated by perl is the same as the one generated by JS (it basically prints the first line that is different if there is one). jQuery is not needed by the main coloration algorythm in coloration.js

Comment 7

[timeless]

Comment on attachment 401293 [details] [diff] [review]
Partial patch, only lines number are added via JS, ~20% space gain only

for js, i tend to use 2 space indentation.

>+window.addCode = function( code )

I don't use spaces inside parens, but i do before them:

function (code)

>+    if( firstTime )

I prefer braces on the same line as if/else

if (firstTime) {

>+            for( var i=0; i<window.marked_lines.length; i++ )

spaces around <

for (var i=0; i < window.marked_lines.length; i++) {

no spaces inside []s
>+                marked_lines[ window.marked_lines[ i ] ] = true;
marked_lines[window.marked_lines[i]] = true;

it's probably a good idea to grab window.marked_lines once at the beginning and store it in a local variable, accessing properties of window is typically very expensive.

i'd prefer if the function bit was on its own line:
>+    code = code.replace( /\n(<div class="m">|<\/div>)*/g, function( str )

code = code.replace(
         /\n(<div class="m">|<\/div>)*/g,
         function (str) {

>@@ -641,42 +642,44 @@ sub build_mark_map {
>+    push( @jscol_selected_lines, $mark );

push @jscol_selected_lines, $mark;

i can't think of any reason for this change:
>-    $class .= ' m' if $mark =~ 's';
>+    $class .= ' m' if $mark =~ 's' ;


>+  if( $jscoloration )
>+  {
>+    return $tag;

return $tag if $jscoloration;

i'm not a great fan of 'jscoloration' how about colorwithjs ?

please stick a blank line before subs ...
>+}
>+sub end_print_code {

i'm sorry about the delay, i was on my summer vacation and then burried under bugmail. i have 1750+ bugmails left after these. you're being served in the order in which things arrived (mostly). the r- is because i want to review it again. please don't take this as a negative, i do like the proposal and design and am thankful for the contribution.

Comment 8

[timeless]

Comment on attachment 401829 [details] [diff] [review]
Final patch, links to identifiers and some common stuff compressed, ~70% total space gain. Includes basic test page.

why is this block commented out...:
>+/*
>+window.onload = function()
>+{
>+    var s = document.createTextNode( allTags.join( "\n" ) );
>+    var pre = document.createElement( "pre" );
>+    pre.appendChild( s );
>+    var body = document.documentElement;
>+    body.appendChild( document.createElement( "hr" ) )
>+    body.appendChild( pre );
>+};
>+*/

the rest of my comments for this file are basically "please use the style I requested in my comments on the previous patch".

>diff -r 7120e385b1ff lib/LXR/Common.pm
>@@ -669,21 +669,17 @@ sub linetag {
>-      my $size = (log10((stat($Path->{'realf'}))[7] / 40) | 0) + 1;

hrm, it occurs to me that it probably wasn't a good idea to stat that file repeatedly, oops :)

please include a space after '#':
>+  #estimate padding

and drop this:
>+  #$padding = 1;

Comment 9

[Jordan Osete]

Attachment: Third Patch - adressing review comments

Again, this is a patch to the patch.
I think I adressed every comment and used the style you requested everywhere.

I also set the cookie path explicitly to '/', and added automatic cookie suppression on page load (perl removes the cookie, then JS sets it again), so that if JS is disabled, reloading the page is enough to get back to "old" JS-less behaviour.

A message is also displayed in a <noscript> tag in that case: "Script has been disabled, please reload the page to see the scriptless version."

check_jscoloration.htm is also more reliable (using a RE to get the code, and not relying on innerHTML exactness).

NP for the delay, and good luck for your remaining bugmails :)

Comment 10

[timeless]

ok, this is currently live on mxr-test.konigsberg.mozilla.org

I'd like to have some testers kick it around a little bit before i push it.

Comment 11

[Jordan Osete]

BTW, for the cookie to be taken into account, you often need to force a refresh (Ctrl-F5) if you have already visited the page (MXR seems to have a very agressive cache policy).

Comment 12

[timeless]

http://hg.mozilla.org/webtools/mxr/rev/0787af51c0ed

I made a few minor changes, there was a problem w/ an empty file, I removed some trailing whitespace, and i removed a 'unicode replacement character' in favor of an octal encoding (I'm assuming this was just damage from an editor).

Thanks for the patch, and I'm sorry about the delay in landing it.