Closed Bug 282083 Opened 16 years ago Closed 13 years ago

can copy_string avoid the loop?

Categories

(Core :: String, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: dwitte)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(1 file, 2 obsolete files)

http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsAlgorithm.h#88

the loop looks like it may be a leftover from multi-fragment strings; since all
strings are single-fragment now, it seems like this could now always be done in
a single call to |write|.

if that's done, it may be possible to make |write| return void, assuming it will
always write all the data (but that may be a bad assumption?)


One problem: It seems that nsScannerString still uses multi-fragment strings.
Would that make this bug INVALID/WONTFIX?
Yes, you have to make sure that nsScannerString has its own overloaded version
of copy_string, or you would want to rename and ensure that it is still using
the multifragment algo.  I think that most of the string stuff was forked for
nsScannerString, but it might still be picking up things like copy_string. 
Someone should take a look at that.
Attached patch patch v1 (diff -w) (obsolete) — Splinter Review
this makes the following optimizations:

1) make copy_string work on a single fragment.
2) constify the |InputIterator& first| argument to copy_string and don't increment it, since no one in the tree actually cares.
3) make write return void, since memmove is predictable.
4) remove copy_string_backward, since there are no consumers.
5) provide a copy_multifragment_string for nsScannerString.
6) update a few functions in nsReadableUtils.cpp to singlefragmentedness.

note that a side-effect of 1) is that copy_string will now call write even if first == last, but memmove will handle a zero-byte copy just fine, and we probably don't want the extra branch for this rare case.

jst, i hope you don't mind if i tag you for this :)
Assignee: string → dwitte
Status: NEW → ASSIGNED
Attachment #294916 - Flags: superreview?
Attachment #294916 - Flags: review?
Attachment #294916 - Flags: superreview?(jst)
Attachment #294916 - Flags: superreview?
Attachment #294916 - Flags: review?(jst)
Attachment #294916 - Flags: review?
Attached patch supplement (obsolete) — Splinter Review
jst pointed out that nsUTF8Utils.h also has some write()s - so i grepped around the tree a bit and found a bunch more. this fixes all of them.

the only two consumers who looked at the rv, in toolkit/xre, can probably do fine without it - for instance, the converter's write() functions returned the entire requested length N even in failure cases, so it doesn't seem all that useful.
Attachment #295163 - Flags: superreview?(jst)
Attachment #295163 - Flags: review?(jst)
Comment on attachment 294916 [details] [diff] [review]
patch v1 (diff -w)

- In IsASCII() (nsReadableUtils.cpp):

-      // for each chunk of |aString|...
-    PRUint32 fragmentLength = 0;
     nsAString::const_iterator iter;
-    for ( aString.BeginReading(iter); iter != done_reading; iter.advance( PRInt32(fragmentLength) ) )
+    for ( aString.BeginReading(iter); iter != done_reading; ++iter )
       {
-        fragmentLength = PRUint32(iter.size_forward());
-        const PRUnichar* c = iter.get();
-        const PRUnichar* fragmentEnd = c + fragmentLength;
-
-          // for each character in this chunk...
-        while ( c < fragmentEnd )
-          if ( *c++ & NOT_ASCII )
+        if ( *iter & NOT_ASCII )
             return PR_FALSE;
       }

This changes the inner-most loop of this code to loop over an iterator rather than simply looping over the PRUinchar*. Maybe a good compiler can generate identical code for the two cases, but I'd bet this would be faster if you got the PRUnichar pointer and the end out of the iterator upfront, and looped directly over that instead. Same thing in the other functions changed in the same file.

- In parser/htmlparser/src/nsScannerString.cpp:

+// private helper function
+template <class InputIterator, class OutputIterator>
+static inline
+OutputIterator&
+copy_multifragment_string( InputIterator& first, const InputIterator& last, OutputIterator& result )
+  {

This is only ever going to be used with one type of arguments right? If so, could we declare them here rather than using templates here?

r- based on the first comments here, just since I think I should look that over once you've made those changes, but the rest look good to me.
Attachment #294916 - Flags: superreview?(jst)
Attachment #294916 - Flags: superreview-
Attachment #294916 - Flags: review?(jst)
Attachment #294916 - Flags: review-
Comment on attachment 295163 [details] [diff] [review]
supplement

 class nsFileCharSink
   {
     public:
       typedef CharT value_type;
 
     public:
       nsFileCharSink( FILE* aOutputFile ) : mOutputFile(aOutputFile) { }
 
-      PRUint32
+      void
       write( const value_type* s, PRUint32 n )
         {
-          return fwrite(s, sizeof(CharT), n, mOutputFile);
+          fwrite(s, sizeof(CharT), n, mOutputFile);
         }

This is a bit scary, as an error here would get lost (not that it wouldn't before this change). But yay for unused code! Wanna file a bug to remove nsFileCharSink while you're here?

r+sr=jst
Attachment #295163 - Flags: superreview?(jst)
Attachment #295163 - Flags: superreview+
Attachment #295163 - Flags: review?(jst)
Attachment #295163 - Flags: review+
nice catch with the iterators - i didn't know they were still storing beginning, position, and end in the single-fragment world. :/

the nsReadableUtils changes are at the end of the diff.
Attachment #294916 - Attachment is obsolete: true
Attachment #295163 - Attachment is obsolete: true
Attachment #295166 - Flags: superreview?(jst)
Attachment #295166 - Flags: review?(jst)
Comment on attachment 295166 [details] [diff] [review]
v2 (diff -w, whole thing)

r+sr=jst!
Attachment #295166 - Flags: superreview?(jst)
Attachment #295166 - Flags: superreview+
Attachment #295166 - Flags: review?(jst)
Attachment #295166 - Flags: review+
Attachment #295166 - Flags: approval1.9?
Attachment #295166 - Flags: approval1.9? → approval1.9+
checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 624410
You need to log in before you can comment on or make changes to this bug.