Add a trivial code generator for the DFA
Created attachment 245577 [details] Patch
Created attachment 245584 [details] Patch
Comment on attachment 245584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245584&action=review I don't feel confident reviewing, someone else should take a look but I made a few (minor) comments. > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:141 > + PotentialPageLoadDescriptor potentialPageLoadDescriptor; PotentialPageLoadDescriptor potentialPageLoadDescriptor{urlCString.data(), urlCString.length()}; > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:145 > + HashSet<unsigned, DefaultHash<unsigned>::Hash, WTF::UnsignedWithZeroKeyHashTraits<unsigned>> triggeredActions; "TriggeredActionSet triggeredActions;" ? > Source/WebCore/contentextensions/DFA.h:50 > + const DFANode& nodeAt(unsigned i) const nit: Could be on a single line. > Source/WebCore/contentextensions/DFA.h:59 > + extra line? > Source/WebCore/contentextensions/DFACompiler.cpp:48 > + DFACodeGenerator(const DFA&); explicit? > Source/WebCore/contentextensions/DFACompiler.cpp:55 > + void getNextCharacter(Assembler::JumpList& failureCases, LocalRegister& output); "get" prefix? > Source/WebCore/contentextensions/DFACompiler.cpp:64 > + JSC::MacroAssembler::RegisterID m_urlBufferOffsetRegister = InvalidGPRReg; Assembler::RegisterID ? > Source/WebCore/contentextensions/DFACompiler.cpp:140 > + callAddActionFunction(static_cast<unsigned>(actionId)); Why is this safe? > Source/WebCore/contentextensions/DFACompiler.cpp:145 > + for (const auto transition : dfaNode.transitions) { auto& transition ? It probably doesn't matter for this particular underlying type if we're copying it but I still like to use auto& in range loops. I would omit the const but this may be a matter of taste.
Comment on attachment 245584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245584&action=review > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:129 > + reinterpret_cast<TriggeredActionSet*>(data)->add(actionId); Could we avoid the reinterpret cast by changing the signature of this function & the AddActionFunction typedef? > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:140 > + CString urlCString = urlString.utf8(); Is converting to Cstring efficient here? – may be good to add a fixme & bugzilla number to remember to revisit.
(In reply to comment #4) > > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:129 > > + reinterpret_cast<TriggeredActionSet*>(data)->add(actionId); > > Could we avoid the reinterpret cast by changing the signature of this > function & the AddActionFunction typedef? Yes, I should do that :) > > Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:140 > > + CString urlCString = urlString.utf8(); > > Is converting to Cstring efficient here? – may be good to add a fixme & > bugzilla number to remember to revisit. I don't expect this to be a big problem. The valid URLS are already ASCII so there is no encoding cost.
Comment on attachment 245584 [details] Patch r+ to move this forward, though please consider the feedback given by Gavin & Chris. :-)
Committed r180260: <http://trac.webkit.org/changeset/180260>
(In reply to comment #7) > Committed r180260: <http://trac.webkit.org/changeset/180260> Seems this broke 32-bit builds? https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-bit%20Build%29/builds/685 https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-bit%20Build%29/builds/685/steps/compile-webkit/logs/errors > Source/WebCore/contentextensions/DFACompiler.cpp:55:69: error: unknown type name 'LocalRegister' > Source/WebCore/contentextensions/DFACompiler.cpp:61:5: error: unknown type name 'RegisterAllocator' > Source/WebCore/contentextensions/DFACompiler.cpp:62:5: error: unknown type name 'StackAllocator'; did you mean 'JSC::BlockAllocator'? > Source/WebCore/contentextensions/DFACompiler.cpp:77:7: error: no matching constructor for initialization of 'JSC::BlockAllocator'
Re-opened since this is blocked by bug 141757
This works on ARM64 with the same prolog and epilog as the cssjit. That code could be shared, but DFACodeGenerator would also need a m_prologueStackReferences. I imagine it would work on ARMv7, too, but I haven't checked that.
We'll use an interpreter instead.