Closed
Bug 74486
Opened 24 years ago
Closed 24 years ago
syntax highlighting is very slow in view source
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: bzbarsky, Assigned: rickg)
References
Details
(Keywords: perf)
Attachments
(8 files)
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
text/plain
|
Details | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review |
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="..."
|
Reporter | |
Comment 1•24 years ago
|
||
|
|
Reporter | |
Comment 2•24 years ago
|
||
|
|
Reporter | |
Comment 3•24 years ago
|
||
er.. that first patch is bogus. attaching a patch that correctly uses <link> to
load an external stylesheet.... Still doesn't work though
|
|
Reporter | |
Comment 4•24 years ago
|
||
|
||
Comment 5•24 years ago
|
||
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...
Cc:ing style people who might provide some helpful pointers about the remaining
probem.
|
|
Reporter | |
Comment 8•24 years ago
|
||
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....
|
|
Reporter | |
Updated•24 years ago
|
Keywords: mozilla1.0
|
||
Comment 10•24 years ago
|
||
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?
|
|
Reporter | |
Comment 11•24 years ago
|
||
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...
|
|
Reporter | |
Comment 12•24 years ago
|
||
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.
|
|
Reporter | |
Comment 13•24 years ago
|
||
|
|
Reporter | |
Comment 14•24 years ago
|
||
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
|
||
Comment 15•24 years ago
|
||
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.
|
|
Reporter | |
Comment 16•24 years ago
|
||
|
|
Reporter | |
Comment 17•24 years ago
|
||
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?
|
|
Reporter | |
Comment 18•24 years ago
|
||
|
|
||
Comment 19•24 years ago
|
||
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').
|
|
Reporter | |
Comment 20•24 years ago
|
||
|
|
||
Comment 21•24 years ago
|
||
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.
|
|
Reporter | |
Comment 22•24 years ago
|
||
|
|
||
Comment 23•24 years ago
|
||
Looks great and ready to go!
r=rbs
|
||
Comment 24•24 years ago
|
||
Chak, can you comment on what effect your view-source: changes may have on this?
|
|
||
Comment 25•24 years ago
|
||
bzbarsky: it just stroke me that you forgot the release in nsLayoutModule.cpp
|
||
Comment 26•24 years ago
|
||
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
|
|
||
Comment 27•24 years ago
|
||
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
|
|
||
Comment 28•24 years ago
|
||
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
|
|
||
Comment 29•24 years ago
|
||
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.
|
|
||
Comment 30•24 years ago
|
||
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">
<<span class="start-tag">HTML</span>>
<<span class="start-tag">BODY</span>>
</<span class="end-tag">BODY</span>>
<<span class="end-tag">HTML</span>>
</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: 24 years ago
Resolution: --- → FIXED
|
|
||
Comment 31•24 years ago
|
||
Made a typo in my checkin log. I meant to write r=rbs sr=attinasi.
|
|
||
Comment 32•24 years ago
|
||
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?
|
|
||
Comment 33•24 years ago
|
||
Never mind. It won't help because it wont exlude anything. All are 'span'...
|
|
||
Comment 34•24 years ago
|
||
It would exclude the BODY and the HTML and the PRE... not much though.
|
||
Comment 35•24 years ago
|
||
|
||
Comment 36•24 years ago
|
||
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.
Description
•