[regexp] A different approach to the not-at-start optimization [v8/v8 : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1041c387310000
Related details
- Erik Corry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Corry (Gerrit)
Erik Corry added 1 comment![]( "Open in Gerrit")
The only one that gets slower is tagcloud-SP from Sunspider. The only regexp that is code generated differently is this regexp:
/(?:^|:|,)(?:\s*\[)+/g
The old code was using quickcheck, where the new one uses a bitmap to find the colon or comma.
Old:
label[00ea8408]: (Bind)
LoadCurrentCharacter(cp_offset=0, label[00000000] (1 chars) (eats at least 1));
CheckCharacterAfterAnd(c=0x0028((), mask=0x00e9, label[f1bd3498]);
label[f1bd34a4]: (Bind)
AdvanceCurrentPosition(by=1);
GoTo(label[00ea8408]);
New:
SkipUntilBitInTable(cp_offset=0, advance_by=1, on_match=label[20ec9540], on_no_match=label[20ec9540]
................................
............X.............X.....
................................
................................
Ironically this is because previously the Boyer-Moore analysis was failing and now it is succeeding.
Also interesting that this regexp got faster on my machine (Intel), but slower on tagcloud (Mac) so perhaps it's about the bit search instructions. On my machine it's not a win to disable Boyer-Moore for this regexp.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11a25abd310000
Related details
- Erik Corry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16d1951cb10000
Related details
- Erik Corry
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10b38617310000
Related details
- Erik Corry
- Jakob Linke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Linke (Gerrit)
Jakob Linke added 17 comments![]( "Open in Gerrit")
[regexp] A different approach to the not-at-start optimizationVery nice how we no longer pass not_at_start around.
It sounds like this is also expected to be a perf improvement. Do you know why we don't see that on pp, and why tagcloud regresses?
if (on_success->IsBacktrack()) return on_success;Also here, as this is quite important please add a comment.
return zone->template New<EndNode>(EndNode::BACKTRACK, zone);Just curious, do you need the `template `? Below are calls that do fine without it.
bool not_at_start() const { return not_at_start_; }Optional: Since you're here, wdyt about flipping this to `at_start` to make conditions easier to read?
assembler->GoTo(trace->backtrack());Please comment why this happens before everything else.
Isn't Bind needed to avoid dcheck failures?
switch (action_) {Suggest: `DCHECK_EQ(action_, ACCEPT); ...`
// This case is handled in a different virtual method.nit: this comment doesn't apply to BACKTRACK.
BoyerMooreLookahead* bm, bool not_at_start) {Very nice!
if (action_ == BACKTRACK) return;CHECK_EQ(action_, BACKTRACK)
int RegExpNode::EatsAtLeast() { return eats_at_least_; }Why the change to int? Maybe uint8_t makes sense, also given the UINT8_MAX constants used elsewhere.
std::min(int{kMaxLookaheadForBoyerMoore}, EatsAtLeast());nit: static_cast, also below.
// We are going to advance backward, so we may end up at the start.How does this new design handle the read_backwards case?
int eats_at_least = i == 0 ? UINT8_MAX : that->eats_at_least_info();nit: the underlying types aren't UINT8 anymore.
if (action == BACKTRACK) set_eats_at_least_info(UINT8_MAX);Also here. Wdyt of a named constant `kMaxEatsAtLeastValue`?
virtual bool IsBacktrack() const { return false; }I was going to suggest `bool Is##type##Node() { return As##type##Node() != nullptr; }`, but I see Backtrack is a special case of EndNode.
int eats_at_least_info() const { return eats_at_least_; }nit: rename to remove the _info? Also the setter.
Related details
- Erik Corry
- Patrick Thier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |