Closed Bug 26384 Opened 20 years ago Closed 18 years ago

need to ensure that NS_LITERAL_STRING is used

Categories

(Core :: String, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: rubydoo123, Assigned: Brade)

Details

(Keywords: helpwanted, perf, Whiteboard: [perf])

Attachments

(1 file, 5 obsolete files)

general tracking/reminder bug for debugging areas to enhance performance and for 
optimization of code.
setting to m16 -- getting the schedule and bugzilla in sync
Target Milestone: M16
Adding perf keyword
Keywords: perf
Status: NEW → ASSIGNED
Moving bugs to M17 to concentrate on features in M16
Target Milestone: M16 → M17
Keywords: nsbeta3
Target Milestone: M17 → M18
moving to m19
Target Milestone: M18 → M19
Keywords: nsbeta3
please do not nominate for nsbeta3 -- this will be addressed after crashes, 
regressions, correctness and polish bugs have been resolved, all Composer perf 
bugs have been set to m19/m20
I think the problem concerning use of NS_LITERAL_STRING in nsHTMLEditor.cpp.
Note that all our cpp files should be examined an NS_LITERAL_STRING used if 
possible.
Assignee: cmanske → brade
Status: ASSIGNED → NEW
updating summary, changing priority to p2
Priority: P3 → P2
Summary: Performance and optimization cleanup → need to ensure that NS_LITERAL_STRING is used
Whiteboard: [p:2]
Keywords: nsbeta3
Whiteboard: [p:2] → [nsbeta3+][p:2]
performance improvement
PDT thinks this is nsbeta3- since no bad behavior is described, just code cleanup
Whiteboard: [nsbeta3+][p:2] → [nsbeta3-][p:2]
pdt deems this not necessary for beta3, setting it to nsbeta3- which places it
below the wire for beta and rtm, marking future and adding helpwanted. If this
impacts your work or impacts another bug, please state your arguement in the
bug, delete entries in the whiteboard for reconsideration.
Keywords: helpwanted
Target Milestone: M19 → Future
my understanding is that this bug being fixed will improve performance but I have 
no numbers to prove that so this will have to wait until after Netscape6 rtm (or 
someone else will have to pick it up).
Status: NEW → ASSIGNED
Keywords: nsbeta3
Hardware: PC → All
Whiteboard: [nsbeta3-][p:2]
This is not specific to the editor, there is code all over that does not use
NS_LITERAL_STRING. But editor has used the inefficient forms perhaps the most,
so editor stands to gain most...

Clearing target milestone and nominating for nsbeta1. I did some measuring,
posted results on netscape.public.mozilla.xpcom. Here are some details from the
post:

Test 1:

We have a loop calling a function that takes const nsAReadableString
&aStr as a parameter. Compare NS_ConvertASCIItoUCS2 performance against
NS_LITERAL_STRING performance, with short and long strings.

Typical run:

Short string
   convert:    46000
   literal:    16000
   =================
   Diff:       30000, literal 34.782609 of convert
Long string
   convert:   469000
   literal:    15000
   =================
   Diff:      454000, literal 3.198294 of convert

NOTE: On 50% of the runs the times for literal were 0.

NOTE: Typically we have short strings, we might in fact never have
strings that are so long that they require allocation in our code base.


Conclusion:

By replacing NS_ConvertASCIItoUCS2 with NS_LITERAL_STRING as an [in] parameter
to a function call, you can cut away at least 65% of time spent in needless
string conversion/manipulation on platforms that support L"string". If you have
performance critical code using this pattern you will certainly notice the effect.
Keywords: nsbeta1
Target Milestone: Future → ---
moving to moz0.9.2
Target Milestone: --- → mozilla0.9.2
I did a little testing to see how much page load times could be affected.
Running jrgm's page load test suite with and without the patch I just attached
(mozilla/layout only) did not produce noticeable change. I think with the patch
the load time might be 0.5% faster, but it mostly gets lost in the noise. And it
is estimated that the error margin in the tool is around 2%.

So unless there is some code that is called many, many times it seems like this
is not a critical issue. However, this is not a difficult task, and does produce
at least a little performance benefits, it should be fixed sooner rather than
later in my opinion. Also, when I fixed content directory I actually found one
buggy interface: a function had nsString& parameter, when it should have been
const nsString& (now const nsAReadableString&).

Based on the observations I am changing this to nsbeta1-. It would be nice if
the nominations were paid attention to when shuffling bugs around...
Keywords: nsbeta1nsbeta1-
the nsbeta1+ would have been entered into the status whiteboard if it was really 
 a beta blocker, since this was never considered to be such an issue, it was 
never accepted as one and tha is why it was moved to 0.9.2
Whiteboard: [perf]
nsbeta1+/- in the status bar has been deprecated since NS 6.0 RTM. The "new"
style is keywords nsbeta1+ and nsbeta1-. I don't know why this was changed. And
in case you were not aware: nsbeta1 keywords were cleared after RTM so all
existing nsbeta1 nominations are new.

When you moved this nsbeta1 nominated bug to 0.9.2 you should have changed the
nsbeta1 keyword to nsbeta1- to indicate that it was not a beta blocker in your
opinion. As it was, you were sending conflicting messages: on one hand saying
that it is not a beta blocker by moving the milestone but at the same time
leaving the nomination there, indicating that it might be a beta blocker after all.

There is a difference between all of the nsbeta1 keywords:

nsbeta1   nominated, don't know if it will be a blocker or not
nsbeta1-  rejected, not a blocker
nsbeta1+  accepted, a blocker

Not that big a deal, but it would be nice if we all followed the same processes.
Target Milestone: mozilla0.9.2 → mozilla1.0
This is low-hanging fruit.  While each change will probably not be noticable,
replacing all unneeded uses is both easy and an obvious improvement.  

If you have a field of nails sticking up, each one you hammer down won't seem to
make a difference - until you've hammered a bunch of them.

Moving to a more general area to get it onto more people's radar.
Component: Editor → String
Would there be any objections if I were to make a patch (or patches) for large
sections of the mozilla tree for switching to NS_LITERAL_STRING?  Any gotcha's
about where not to make the switch?  This would be the hitlist to investigate:

http://lxr.mozilla.org/seamonkey/ident?i=NS_ConvertASCIItoUCS2

(Many of those are fine, many others could be converted)
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
Target Milestone: mozilla1.0.1 → mozilla0.9.7
Attached patch patch (obsolete) — Splinter Review
patch for files in mozilla/editor
Comment on attachment 60671 [details] [diff] [review]
patch

sr=kin@netscape.com

Just lose the commented out includes in nsEditorShell.cpp.
Attachment #60671 - Flags: superreview+
... and where applicable change the pattern

  nsAutoString str("foo"); str.Assign(NS_LITERAL_STRING("foo"));

to set the value during the construction:

  nsAutoString str(NS_LITERAL_STRING("foo"));

sr=kin@netscape.com with those changes plus any others akkana wants. :-)

My only issue was the one Kin mentioned about assiging the nsAutoStrings.  Other
than that, r=akkana.  Thanks for doing this!
I checked in all of the fixes for mozilla/editor today (except one file which
has some other changes in it).
I'm going to add one thing - if you have code that does this:

nsAutoString foo(NS_LITERAL_STRING("foo"));

but you never actually change the value of the variable foo after that, you can
instead say
NS_NAMED_LITERAL_STRING(foo, "foo");

and that will prevent an extra allocation + copy
Woohoo! Thanks brade!

I've got another bug for this which I guess I could dup on this one. I have a
big patch for this, which you've made a nice bit smaller, I'll attach it here as
soon as I know it compiles on all three platforms.
Attached patch fixes based on alecf's comments (obsolete) — Splinter Review
ignore #ifdef stuff
Attachment #13762 - Attachment is obsolete: true
Attachment #33119 - Attachment is obsolete: true
Attachment #60671 - Attachment is obsolete: true
Comment on attachment 61313 [details] [diff] [review]
fixes based on alecf's comments

r=cmanske on all the NS_LITERAL_STRING and NS_NAMED_LITERAL_STRING changes.
Attachment #61313 - Flags: review+
Comment on attachment 61313 [details] [diff] [review]
fixes based on alecf's comments

>Index: composer/src/nsComposerCommands.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/editor/composer/src/nsComposerCommands.cpp,v
>retrieving revision 1.35
>diff -u -2 -r1.35 nsComposerCommands.cpp
>--- composer/src/nsComposerCommands.cpp	7 Dec 2001 15:16:27 -0000	1.35
>+++ composer/src/nsComposerCommands.cpp	11 Dec 2001 15:23:14 -0000
>@@ -511,5 +511,5 @@
>   if (editorShell)
>   {
>-    nsAutoString indentStr(NS_LITERAL_STRING("indent"));
>+    NS_NAMED_LITERAL_STRING(indentStr, "indent");
>     rv = editorShell->Indent(indentStr.get());
>   }
>@@ -549,5 +549,5 @@
>   if (editorShell && EditingHTML(editorShell))
>   {
>-    nsAutoString indentStr(NS_LITERAL_STRING("outdent"));
>+    NS_NAMED_LITERAL_STRING(indentStr, "outdent");
>     rv = editorShell->Indent(indentStr.get());
>   }

Actually, for cases like these you may just want to inline them, so:

   if (editorShell)
   {
     rv = editorShell->Indent(NS_LITERAL_STRING("indent").get());
   }

   if (editorShell && EditingHTML(editorShell))
   {
     rv = editorShell->Indent(NS_LITERAL_STRING("outdent").get());
   }

Optimally, you would make Indent accept nsAString& so you can get rid of
that .get() (since there's likely to be something on the other end that needs
to know the length or convert it back to some string class anyway.

Heh, yep. |nsAutoString aIndent(indent);| in nsEditorShell::Indent.

You should probably deal with that in another bug, but at least inline
these NS_LITERAL_STRINGs.
again, ignore any code removal or #ifdefs since that is part of another bug
Attachment #61313 - Attachment is obsolete: true
alecf: please sr=
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Comment on attachment 61610 [details] [diff] [review]
patch 2 from alecf's / jag's feedback

r=cmanske
Attachment #61610 - Flags: review+
Comment on attachment 61610 [details] [diff] [review]
patch 2 from alecf's / jag's feedback

awesome!
sr=alecf
Attachment #61610 - Flags: superreview+
Apparently mjudge recently checked in a bunch of commander code which doesn't
use NS_LITERAL_STRINGS.  We should fix this since we need the commander code to
be as optimized as possible.
Keywords: nsbeta1+
Target Milestone: mozilla0.9.8 → mozilla0.9.9
obsoleting previous patch since it was checked in
new patch addresses regressions checked in by mjudge
reviews please?
Attachment #61610 - Attachment is obsolete: true
Comment on attachment 70382 [details] [diff] [review]
fix mjudge's checkin which introduced more non-literal string conversion

r=cmanske
Attachment #70382 - Flags: review+
Comment on attachment 70382 [details] [diff] [review]
fix mjudge's checkin which introduced more non-literal string conversion

sr=kin@netscape.com
Attachment #70382 - Flags: superreview+
fix checked in
Sujay--you can go ahead and verify this per my comments

Any new regressions should be dealt with in a new bug.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.