[webkit-reviews] review granted: [Bug 228301] [JSC] Yarr should perform BoyerMoore search : [Attachment 434488] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 228301: [JSC] Yarr should perform BoyerMoore search
https://bugs.webkit.org/show_bug.cgi?id=228301

Attachment 434488: Patch

https://bugs.webkit.org/attachment.cgi?id=434488&action=review




--- Comment #21 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 434488
  --> https://bugs.webkit.org/attachment.cgi?id=434488
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434488&action=review

r=me

I also suggest filing/linking bugs for your FIXMEs

> Source/JavaScriptCore/yarr/YarrJIT.cpp:944
> +	   BoyerMooreInfo* m_bmInfo;

can we just default this to nullptr so we don't need to set it at the call
sites

> Source/JavaScriptCore/yarr/YarrJIT.cpp:2415
> +			       auto vector =
makeUniqueRef<BoyerMooreByteVector>(map);
> +			       dataLogLnIf(Options::verboseRegExpCompilation(),
"Found bitmap lookahead count:(", mapCount, "),range:[", beginIndex, ", ",
endIndex, ")");
> +			       const uint8_t* pointer = vector->data();
> +			       if (const auto* existingPointer =
m_codeBlock.findSameVector(op.m_bmInfo->index(), vector.get()))
> +				   pointer = existingPointer;
> +			       else
> +				   m_bmVector.append(WTFMove(vector));

small nit: I think it's nicer to encapsulate this logic in its own function, so
we don't mistakenly use a stale pointer somehow. Like, "getBMByteVector" or
something, and it take in BoyerMooreBitmap::Map&

> Source/JavaScriptCore/yarr/YarrJIT.h:127
> +    void set8BitCode(MacroAssemblerCodeRef<Yarr8BitPtrTag> ref,
Vector<UniqueRef<BoyerMooreByteVector>>&& maps)

this should take Vector& not Vector&& unless it actually takes ownership over
"maps". Alternatively, it can take "Vector", and take ownership

> Source/JavaScriptCore/yarr/YarrJIT.h:134
> +    void set16BitCode(MacroAssemblerCodeRef<Yarr16BitPtrTag> ref,
Vector<UniqueRef<BoyerMooreByteVector>>&& maps)

ditto

> Source/JavaScriptCore/yarr/YarrJIT.h:144
> +    void set8BitCodeMatchOnly(MacroAssemblerCodeRef<YarrMatchOnly8BitPtrTag>
matchOnly, Vector<UniqueRef<BoyerMooreByteVector>>&& maps)

ditto

> Source/JavaScriptCore/yarr/YarrJIT.h:151
> +    void
set16BitCodeMatchOnly(MacroAssemblerCodeRef<YarrMatchOnly16BitPtrTag>
matchOnly, Vector<UniqueRef<BoyerMooreByteVector>>&& maps)

ditto

> Source/JavaScriptCore/yarr/YarrJIT.h:243
> +    // Compile and Clear are guarded by RegExp's cell-lock. So these
operations work atomically.

nit: would be nicer if they just took a const Locker& instead of a comment

> Source/JavaScriptCore/yarr/YarrJIT.h:254
> +    const uint8_t* findSameVector(unsigned index, BoyerMooreByteVector&
vector) const

tryFindSameBoyerMooreVector? or tryFindSameBMVector? or tryReuseBMVector?

findSameVector is a weirdly generic name