Closed Bug 74486 Opened 19 years ago Closed 19 years ago

syntax highlighting is very slow in view source

Categories

(Core :: DOM: HTML Parser, defect)

x86
All
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: rickg)

References

Details

(Keywords: perf)

Attachments

(8 files)

I was looking over the syntax highlighting code and two perf hits jumped out at me:

1)  We fetch the highlighting pref for every token we emit.  There is no reason
to do this.

2)  We use <span style="..."> to do highlighting.  This is very slow.  It would
be better to use <span class="...">

I've got a patch that solves the first problem and nearly solves the second
problem.  "Nearly", because I can't figure out how to load a stylesheet for the
view-source document.  My attempt to insert a <style> node into the document
seems to not be working....  If I place the corresponding rules in my
userContent.css, I get syntax highlighting and I get a perceptible speedup due
to the use of class="..." instead of style="..."
er.. that first patch is bogus.  attaching a patch that correctly uses <link> to
load an external stylesheet....  Still doesn't work though
btw, i helped him test these, and I recieved a roughly 50% speedup (5-6 seconds
for cnn source without, wiht ~3 seconds. Without highlighting it is ~1 second)

Keywords: perf
could help, since bug 52154 has r/sr= , so syntax hilightning will probably be
enabled by default very soon...
Depends on: 52154
Cc:ing style people who might provide some helpful pointers about the remaining
probem.
OK..  I found a way to get at the DOM of view source in JS, so I can actually
see what it looks like.

the <link> node is there in <body> and has the right attribute values. 
document.styleSheets is empty, however.

I'm not sure how to write the <link> node into <head>, since at the point when
the HEAD token would need to be created (after HTML is created but before BODY
is created) we do not have a value for mTokenizer.  In other words, I can create
a <link> node in <head>, but don't know how to set attributes on it....  I am
not sure that putting it inside <head> instead of inside <body> would help....

Adding the <link> node into the DOM with JS doesn't work either, most likely due
to bug 7515....
Keywords: mozilla1.0
Why not put the rules in the file viewsource.css (which already exists, btw, but
is unused) and import it into ua.css?

Actually, I'm a bit mystified because I had no idea that we had the ability to
'turn on' syntax highlighting at all in viewsource. Looking at the rules in
viewsource.css, it seems that there was a lot of effort to support that, but I
wonder if the new scheme is orthogonal to what I see in viewsource.css.

Anyway, if you have a set of predefined classes, and a specific way you want
them styled, then put the ruls into a css file and import it into ua.css. If we
want to have the ability to disable the highlighting you can do it by either
disabling that stylesheet, or by simply changing the classes you assign to the
containers inthe viewsource document. Does this make sense?
Marc, I had the same thought earlier this morning... :)

The existing viewsource.css is from an orthogonal implementation, as far as I
know.  But I could certainly use it.

I have this working completely except for one thing -- that stylesheet needs to
be disabled if we are loading normal web content.  Working on that right now. 
Should hopefully attach a patch in the very near future...
OK, I have it working.  Make sure to apply the patch for bug 74728 as well
before testing this.  :)  (or just pull from after pchen's checkin).

Patch attached.  This includes the necessary build system changes as well.
Blocks: 62678
OK.  So one thing about that patch.  We currently enable/disable the viewsource
stylesheet for every LoadURI call.  This is suboptimal.

What we need to do is to disable it by default and then enable it if needed when
SetViewMode is called.  But I am not sure where to put the default disabling...
 When we are in nsDocShell::Create() we don't yet have a presshell that we can
use... :(  Adam, any suggestions?
No longer blocks: 62678
Blocks: 62678
The "#ifdef VIEW_SOURCE_COLORING" seems useless now and can be removed since all
the other fragments are added with the assumption of coloring with CSS.
And there is now a pref to disable/enable.
OK.  I have yet another patch for this.  This one I actually sort of like.

1)  Add the style rules to viewsource.css
2)  Change nsViewSourceHTML to use classes
3)  Change nsLayoutDLF to turn the stylsheet on and off

This patch should continue working even when bug 66334 if fixed and the docshell
modes go away.

reviews anyone?
Depends on: 66334
Keywords: patch, review
Comments:
I suggest to follow the same pattern that is used for gUAStyleSheet, i.e,
1) use a define (like UA_CSS_ ...)
2) declare gViewSourceStyleSheet (like gUAStyleSheet) in nsLayoutModule.h/cpp
There is only one such module during the life-time of Mozilla, so you will not
have to worry about the gIntances stuff that you added. And you only have to
create the gViewSourceStyleSheet when GetViewSourceStyleSheet() is null in the
first "view-source" mode, and don't have to worry about refcounting the nsURI 
stuff.

Some nits:
- Avoid adding the (constant) parameter uaSheet, and simply find out what it is
in the body on the function.
+nsresult nsLayoutDLF::EnableViewSourceStyleSheet(nsICSSStyleSheet* uaSheet, 
PRBool aEnable)
- Use nsnul in a consistent way (instead of occasionally mixing with '0').
Some further simplifications (in 'dubious hand-made pseudo-diff' for clarity):

NS_IMETHODIMP
nsLayoutDLF::CreateInstance(const char *aCommand,
                            nsIChannel* aChannel,
                            nsILoadGroup* aLoadGroup,
                            const char* aContentType, 
                            nsISupports* aContainer,
                            nsISupports* aExtraInfo,
                            nsIStreamListener** aDocListener,
                            nsIContentViewer** aDocViewer)
{
  nsresult rv = NS_OK;
  if (!GetUAStyleSheet()) {
    // Load the UA style sheet
-   nsCOMPtr<nsIURI> uaURL;  
+   nsCOMPtr<nsIURI> uri; [with the replace propagated...]
    rv = NS_NewURI(getter_AddRefs(uri), UA_CSS_URL);
    if (NS_SUCCEEDED(rv)) {
      nsCOMPtr<nsICSSLoader> cssLoader(do_CreateInstance(kCSSLoaderCID,&rv));
      if (cssLoader) {
        PRBool complete;
        rv = cssLoader->LoadAgentSheet(uri, nsLayoutModule::gUAStyleSheet,
                                       complete, nsnull);
+       if (NS_SUCCEEDED(rv)) {
[errors are not fatal here...]
+         // Also cache the view source stylesheet
+         if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri),VIEW_SOURCE_CSS_URL))){
+           PRBool bHasSheet = PR_FALSE;
+           nsLayoutModule::gUAStyleSheet->
+                           ContainsStyleSheet(uri, bHasSheet,
+                           &nsLayoutModule::gViewSourceStyleSheet))
+           NS_ASSERTION(nsLayoutModule::gViewSourceStyleSheet,
+                 "ViewSourceSheet must be set: ContainsStyleSheet is hosed");
+         }
+       }
      }
    }
    if (NS_FAILED(rv)) {
  #ifdef DEBUG
      printf("*** open of %s failed: error=%x\n", UA_CSS_URL, rv);
  #endif
      return rv;
    }
  }
...
}

[This way, the static GetViewSourceStyleSheet() function could go away, and 
there is no more 'if GetViewSourceStyleSheet()' for each CreateInstance().]

Finally, some protection since there could have been a failure earlier:

+#ifdef DEBUG
+    printf( "Enabling View Source StyleSheet\n");
+#endif
     if (nsLayoutModule::gViewSourceStyleSheet)
+      nsLayoutModule::gViewSourceStyleSheet->SetEnabled(PR_TRUE);
     }
+  } else {
+#ifdef DEBUG
+    printf( "Disabling View Source StyleSheet\n");
+#endif
     if (nsLayoutModule::gViewSourceStyleSheet)
+      nsLayoutModule::gViewSourceStyleSheet->SetEnabled(PR_FALSE);
     }

With these, r=rbs.
Looks great and ready to go!
r=rbs
Chak, can you comment on what effect your view-source: changes may have on this?
bzbarsky: it just stroke me that you forgot the release in nsLayoutModule.cpp
Adam Lock wrote:
> Chak, can you comment on what effect your view-source: changes may have on 
this?

Probably none since i do not touch any of this colorization code. All the 
"view-source:" protocol handler does is to enable "view-source:" type URLs, for 
ex, in the URL bar. It also gets rid of docshell's viewmode attribute.

More info on this at http://bugzilla.mozilla.org/show_bug.cgi?id=66334
Cc:ing a request for sr that I sent to marc. Needless to say, other s-reviewers
are welcome too :-)

-------- Original Message --------
Subject: Bug 74486 - syntax highlighting is very slow in view source
Date: Sat, 14 Apr 2001 17:26:18 +1000
From: "Roger B. Sidje" <rbs@maths.uq.edu.au>
To: attinasi@netscape.com
CC: bzbarsky@mit.edu

Marc, want to sr bzbarsky's patch on bug 74486?
There is a trailing release to add in nsLayoutModule.cpp, but that should be
a no brainer now:

  NS_IF_RELEASE(gUAStyleSheet);
+ NS_IF_RELEASE(gViewSourceStyleSheet);

I have read the pointer provided by chak (in fact, I read that bug before
giving my r). I don't anticipate a problem, and will be on the backup
if problems develop. The gained speed and the fact that colors are not
hard-coded, allowing people to specify their preferred colors make the fix
all the more appealing.
---
RBS
These changes look good. The viewsource.css file needs to have the HTML
namespace though, instead of the viewsource namespace. This works now because of
a bug where rules with an implied universal selector (like
'.view-source-start-tag {...}') are mathing ALL namespaces (bug 35847) but this
is fixed now and will be checked in today or tomorrow, at which time the rules
in viewsource.css will no longer work as they are in the wrong namespace. With
that change, r/sr=attinasi
Thanks marc, I will update my tree to the tip and carry on with this (there
have been some changes since this patch was laying there, e.g., bug 40772).
BTW, I will be doing a 's/view-source-//g' prior checkin, otherwise this prefix
appears on each and every viewsource tag and impacts on the viewsource filesize.
Checked-in. If anyone wants to iterate, here is why syntax hightlighting 
is slow. The coloring is achieved by inserting <span class="something> on
each relevant substring. For example, the about:blank document:
<HTML>
  <BODY>
  </BODY>
</HTML>

is transformed as follows (here each class="." stands for the appropriate
coloring class in viewsource.css):

<html><body>
<pre class="viewsource">

&lt;<span class="start-tag">HTML</span>&gt;
  &lt;<span class="start-tag">BODY</span>&gt;
  &lt;/<span class="end-tag">BODY</span>&gt;
&lt;<span class="end-tag">HTML</span>&gt;

</pre>
</html></body>

Add to this the niceties involved with coloring attributes: name="value", and
the version that is handled internally is quite mouthful (in the DOM sense, the
HTML typesetting is not there). And as if that wasn't enough, lots of style
resolutions are going on. This could scale exponentially on some large
documents. If you got some further ideas to address this problem, feel free
to take up the challenge.

For now, I am resolving this as FIXED. Thanks to bzbarsky for this patch which
provided notable improvements.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Made a typo in my checkin log. I meant to write r=rbs sr=attinasi.
A quick thought. *All* the viewsource coloring style rules are confined to
<span class="something">. Marc, would that make a significant difference if the
'span' tag is explicitly specified in the CSS rules in viewsource.css?
Never mind. It won't help because it wont exlude anything. All are 'span'...
It would exclude the BODY and the HTML and the PRE... not much though.
I just filed two bugs on small problems introduced by the patch:

bug 76531 view source uses hard-coded pixel font size
bug 76567 load viewsource.css from view source window instead of on startup

Btw view source is faster now, thanks :)
Marking verified as per the above developer comments.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.