rubydoo123 Reporter
	Description • 25 years ago

general tracking/reminder bug for debugging areas to enhance performance and for 
optimization of code.

	rubydoo123 Reporter
	Comment 1 • 25 years ago

setting to m16 -- getting the schedule and bugzilla in sync

	leger
	Comment 2 • 25 years ago

Adding perf keyword

	Charles Manske
	Comment 3 • 24 years ago

Moving bugs to M17 to concentrate on features in M16

	rubydoo123 Reporter
	Comment 4 • 24 years ago

moving to m19

	rubydoo123 Reporter
	Comment 5 • 24 years ago

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

	Charles Manske
	Comment 7 • 24 years ago

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.

	rubydoo123 Reporter
	Comment 8 • 24 years ago

updating summary, changing priority to p2

	rubydoo123 Reporter
	Comment 9 • 24 years ago

performance improvement

	Phil Peterson
	Comment 10 • 24 years ago

PDT thinks this is nsbeta3- since no bad behavior is described, just code cleanup

	rubydoo123 Reporter
	Comment 11 • 24 years ago

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.

	Kathleen :Brade Assignee
	Comment 12 • 24 years ago

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).

	Heikki Toivonen (remove -bugzilla when emailing directly)
	Comment 13 • 23 years ago

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.

	rubydoo123 Reporter
	Comment 14 • 23 years ago

moving to moz0.9.2

	Heikki Toivonen (remove -bugzilla when emailing directly)
	Comment 16 • 23 years ago

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...

	rubydoo123 Reporter
	Comment 17 • 23 years ago

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

	Heikki Toivonen (remove -bugzilla when emailing directly)
	Comment 18 • 23 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 19 • 23 years ago

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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 20 • 23 years ago

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)

	Asa Dotzler [:asa]
	Comment 21 • 23 years ago

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)

	Kathleen :Brade Assignee
	Comment 22 • 23 years ago

patch for files in mozilla/editor

	kinmoz
	Comment 23 • 23 years ago

Comment on attachment 60671 [details] [diff] [review]
patch

sr=kin@netscape.com

Just lose the commented out includes in nsEditorShell.cpp.

	kinmoz
	Comment 24 • 23 years ago

... 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. :-)

	Akkana Peck
	Comment 25 • 23 years ago

My only issue was the one Kin mentioned about assiging the nsAutoStrings.  Other
than that, r=akkana.  Thanks for doing this!

	Kathleen :Brade Assignee
	Comment 26 • 23 years ago

I checked in all of the fixes for mozilla/editor today (except one file which
has some other changes in it).

	Alec Flett
	Comment 27 • 23 years ago

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

	jag (Peter Annema)
	Comment 28 • 23 years ago

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.

	Kathleen :Brade Assignee
	Comment 29 • 23 years ago

ignore #ifdef stuff

	Charles Manske
	Comment 30 • 23 years ago

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.

	jag (Peter Annema)
	Comment 31 • 23 years ago

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.

	Kathleen :Brade Assignee
	Comment 32 • 23 years ago

again, ignore any code removal or #ifdefs since that is part of another bug

	Kathleen :Brade Assignee
	Comment 33 • 23 years ago

alecf: please sr=

	Charles Manske
	Comment 34 • 23 years ago

Comment on attachment 61610 [details] [diff] [review]
patch 2 from alecf's / jag's feedback

r=cmanske

	Alec Flett
	Comment 35 • 23 years ago

Comment on attachment 61610 [details] [diff] [review]
patch 2 from alecf's / jag's feedback

awesome!
sr=alecf

	Kathleen :Brade Assignee
	Comment 36 • 23 years ago

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.

	Kathleen :Brade Assignee
	Comment 37 • 23 years ago

obsoleting previous patch since it was checked in
new patch addresses regressions checked in by mjudge
reviews please?

	Charles Manske
	Comment 38 • 23 years ago

Comment on attachment 70382 [details] [diff] [review]
fix mjudge's checkin which introduced more non-literal string conversion

r=cmanske

	kinmoz
	Comment 39 • 23 years ago

Comment on attachment 70382 [details] [diff] [review]
fix mjudge's checkin which introduced more non-literal string conversion

sr=kin@netscape.com

	Kathleen :Brade Assignee
	Comment 40 • 23 years ago

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.

	sujay
	Comment 41 • 23 years ago

verified.