54714 - need a version of Substring() that takes iterators

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Chris Waterson]

Currently, Substring() is set up to take an index and a length. We should have a
version that takes iterators, as well.

Justification: I'm doing performance work on mail scrolling, and the code that
does RDF text substitution (nsXULTemplateBuilder::ParseAttribute) is currently
forced to use indices instead of iterators because there is no Substring()
that'll accept iterators. Using indices to get at the string data is slightly
less efficient than just dereferencing an iterators. Using iterators would allow
me to get about a 25% speedup in nsXULTemplateBuilder::ParseAttribute, which
translates to a 2-3% speedup in mail scrolling performance.

Comment 1

[Chris Waterson]

scc, if you could whip up some patches, I can try it and verify the performance
improvements.

Comment 2

[Scott Collins]

Here is the discussion Chris and I just had on IRC:
..........
3:16 PM:        scc: ok, here's the situation
3:16 PM:        scc: on both the trunk and branch, |nsPromiseSubstring| has
indexes into its target string
3:17 PM:        scc: and is constructed from integers
3:17 PM:   waterson: right
3:17 PM:        scc: and that's wrong and bad, and inefficient for
multi-fragment strings
3:17 PM:        scc: we actually want the index based version to totally go away
in favor of an iterator version
3:17 PM:        scc: so I have that in one of my trees here
3:18 PM:        scc: however
3:18 PM:        scc: in this version, substrings no longer work over
multi-fragment strings
3:18 PM:   waterson: on purpose?
3:19 PM:        scc: no
3:19 PM:        scc: because I haven't done the typing yet (see the top priority
on my Journal page)
3:19 PM:   waterson: no problem
3:19 PM:        scc: so
3:19 PM:   waterson: so don't let me get your priorities all spun around
3:19 PM:        scc: I could give you something quick and dirty that constructed
the current form of substring but from iterators
3:20 PM:   waterson: i'm in the thick of trying to figure out why mail scrolling
is so bad
3:20 PM:        scc: but that would have to do a |Distance| calculation
3:20 PM:   waterson: yeah, that'd be worse
3:20 PM:        scc: well
3:20 PM:        scc: you're not using multi-fragment strings, so probably not
3:20 PM:        scc: it's just something to be aware of
3:20 PM:   waterson: ah
3:20 PM:        scc: since you're _explicitly_ doing this for performance
3:21 PM:        scc: and the |Distance| function exists only in a patch, as well :-)
3:21 PM:   waterson: well, the Substring() call is not what's killing me
3:21 PM:   waterson: it's the CharAt()'s
3:21 PM:        scc: oy veh
3:21 PM:        scc: why are you calling |CharAt|?
3:21 PM:   waterson: my ass-umption is that by replacing with an iterator
3:21 PM:   waterson: that just turns into a ptr deref
3:22 PM:        scc: it does
3:22 PM:        scc: ok
3:22 PM:        scc: so I should give you hacked up |Substring|
3:22 PM:   waterson: gotta keep my place in the string, only way to do that
compatible with Substring() is indicies
3:22 PM:        scc: right
3:22 PM:        scc: ok, I'm on it
3:23 PM:        scc: I think I should copy this chat and append it to the bug by
way of explanation, though
3:23 PM:   waterson: ok3:16 PM:        scc: ok, here's the situation
3:16 PM:        scc: on both the trunk and branch, |nsPromiseSubstring| has
indexes into its target string
3:17 PM:        scc: and is constructed from integers
3:17 PM:   waterson: right
3:17 PM:        scc: and that's wrong and bad, and inefficient for
multi-fragment strings
3:17 PM:        scc: we actually want the index based version to totally go away
in favor of an iterator version
3:17 PM:        scc: so I have that in one of my trees here
3:18 PM:        scc: however
3:18 PM:        scc: in this version, substrings no longer work over
multi-fragment strings
3:18 PM:   waterson: on purpose?
3:19 PM:        scc: no
3:19 PM:        scc: because I haven't done the typing yet (see the top priority
on my Journal page)
3:19 PM:   waterson: no problem
3:19 PM:        scc: so
3:19 PM:   waterson: so don't let me get your priorities all spun around
3:19 PM:        scc: I could give you something quick and dirty that constructed
the current form of substring but from iterators
3:20 PM:   waterson: i'm in the thick of trying to figure out why mail scrolling
is so bad
3:20 PM:        scc: but that would have to do a |Distance| calculation
3:20 PM:   waterson: yeah, that'd be worse
3:20 PM:        scc: well
3:20 PM:        scc: you're not using multi-fragment strings, so probably not
3:20 PM:        scc: it's just something to be aware of
3:20 PM:   waterson: ah
3:20 PM:        scc: since you're _explicitly_ doing this for performance
3:21 PM:        scc: and the |Distance| function exists only in a patch, as well :-)
3:21 PM:   waterson: well, the Substring() call is not what's killing me
3:21 PM:   waterson: it's the CharAt()'s
3:21 PM:        scc: oy veh
3:21 PM:        scc: why are you calling |CharAt|?
3:21 PM:   waterson: my ass-umption is that by replacing with an iterator
3:21 PM:   waterson: that just turns into a ptr deref
3:22 PM:        scc: it does
3:22 PM:        scc: ok
3:22 PM:        scc: so I should give you hacked up |Substring|
3:22 PM:   waterson: gotta keep my place in the string, only way to do that
compatible with Substring() is indicies
3:22 PM:        scc: right
3:22 PM:        scc: ok, I'm on it
3:23 PM:        scc: I think I should copy this chat and append it to the bug by
way of explanation, though
3:23 PM:   waterson: ok

Comment 3

[Scott Collins]

OK, first, to make this work, you'll need my distance patch which you can find
in bug #52721 in my comment of 2000-09-15 19:33 (note Vidur's next comment as
well).  This function really belongs in "nsReadableUtils.h", I think.  I'll make
sure it's included in the patch I attach to this bug.

Comment 4

[Scott Collins]

Attachment: here's the patch as discussed

Comment 5

[Robert John Churchill]

Hey, r=rjc

Comment 6

[Chris Waterson]

sr=waterson

Comment 7

[Scott Collins]

fix checked in on the trunk ... I imagine this is never going over to the branch.  
Waterson: re-open this if you need it on the branch.