Closed
Bug 1018893
Opened 7 years ago
Closed 7 years ago
Latin1 strings: support indexOf and friends
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(3 files)
5.85 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
16.42 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
7.89 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
indexOf/lastIndexOf/contains/startsWith/endsWith are all pretty similar and straight-forward to convert.
Assignee | ||
Comment 1•7 years ago
|
||
Moves the algorithm itself into a new templatized function. I also added some asserts and cleaned up the code a bit. Note that using separate TextChar and PatChar "types" instead of jschar for both actually makes the algorithm a bit more readable.
Attachment #8432442 -
Flags: review?(luke)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8432460 -
Flags: review?(luke)
![]() |
||
Comment 3•7 years ago
|
||
Comment on attachment 8432442 [details] [diff] [review] Part 1 - lastIndexOf Great, and nice tidying.
Attachment #8432442 -
Flags: review?(luke) → review+
![]() |
||
Comment 4•7 years ago
|
||
Comment on attachment 8432460 [details] [diff] [review] Part 2 - indexOf and contains Review of attachment 8432460 [details] [diff] [review]: ----------------------------------------------------------------- srsly, templates ftw ::: js/src/jsstr.cpp @@ +1010,5 @@ > static const int sBMHBadPattern = -2; /* return value if pat is not ISO-Latin-1 */ > > +template <typename TextChar, typename PatChar> > +static int > +BoyerMooreHorspool(const TextChar *text, uint32_t textlen, const PatChar *pat, uint32_t patlen) textLen and patLen? @@ +1015,4 @@ > { > uint8_t skip[sBMHCharSetSize]; > > JS_ASSERT(0 < patlen && patlen <= sBMHPatLenMax); Since you're tidying up, I'd move this assertion to the top and move 'skip' to the the line before the for loop. @@ +1020,4 @@ > for (uint32_t i = 0; i < sBMHCharSetSize; i++) > + skip[i] = uint8_t(patlen); > + > + uint32_t patlast = patlen - 1; patLast? @@ +1121,2 @@ > static MOZ_ALWAYS_INLINE int > +StringMatch(const TextChar *text, uint32_t textlen, const PatChar *pat, uint32_t patlen) textLen, patLen? @@ +1167,2 @@ > * > * FIXME: Linux memcmp performance is sad and the manual loop is faster. It'd be nice to retest this or just remove the hack and see if anything regresses. @@ +1169,5 @@ > */ > return > #if !defined(__linux__) > + (patlen > 128 && IsSame<TextChar, PatChar>::value) > + ? UnrolledMatch<MemCmp<TextChar, PatChar>>(text, textlen, pat, patlen) I'd try server to see if we can depend on C++11 >> (instead of > >).
Attachment #8432460 -
Flags: review?(luke) → review+
Assignee | ||
Comment 5•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe6b7f69763 https://hg.mozilla.org/integration/mozilla-inbound/rev/dcfd3a026c6f (In reply to Luke Wagner [:luke] from comment #4) > srsly, templates ftw Yes! Really nice. > textLen and patLen? I did a search-and-replace in jsstr.cpp (and checked the result). > Since you're tidying up, I'd move this assertion to the top and move 'skip' > to the the line before the for loop. Done. > patLast? Done. > It'd be nice to retest this or just remove the hack and see if anything > regresses. I'll file a follow-up bug. > I'd try server to see if we can depend on C++11 >> (instead of > >). I remember reading a bugzilla comment saying this is okay to use this now, and some grepping shows a lot of places in dom/.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8432711 -
Flags: review?(luke)
Assignee | ||
Comment 7•7 years ago
|
||
Oh this patch also adds a latin1 jit-test directory and moves the basic/latin1-* tests there. The review interface doesn't show that, looks like.
![]() |
||
Comment 8•7 years ago
|
||
Comment on attachment 8432711 [details] [diff] [review] Part 3 - startsWith and endsWith Review of attachment 8432711 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsstr.cpp @@ +1571,5 @@ > + return true; > +} > + > +static bool > +IsSubstring(JSLinearString *text, JSLinearString *pat, size_t start) Perhaps IsSubstringAt hor HasSubstringAt? IsSubstring sounds like something that would behave more like indexOf.
Attachment #8432711 -
Flags: review?(luke) → review+
Comment 9•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3fe6b7f69763 https://hg.mozilla.org/mozilla-central/rev/dcfd3a026c6f
Flags: in-testsuite+
Assignee | ||
Comment 10•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1d94fe525c (In reply to Luke Wagner [:luke] from comment #8) > Perhaps IsSubstringAt hor HasSubstringAt? IsSubstring sounds like something > that would behave more like indexOf. Ah, nice, I couldn't think of a good name. I changed it to HasSubstringAt.
Keywords: leave-open
Comment 11•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ff1d94fe525c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•