Bug 141017 – Add a trivial code generator for the DFA

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Benjamin Poulain]

Add a trivial code generator for the DFA

[Benjamin Poulain]

Created attachment 245577 [details]
Patch

[Benjamin Poulain]

Created attachment 245584 [details]
Patch

[Chris Dumez]

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.

[Gavin Barraclough]

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.

[Benjamin Poulain]

(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.

[Andreas Kling]

Comment on attachment 245584 [details]
Patch

r+ to move this forward, though please consider the feedback given by Gavin & Chris. :-)

[Benjamin Poulain]

Committed r180260: <http://trac.webkit.org/changeset/180260>

[Joseph Pecoraro]

(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'

[WebKit Commit Bot]

Re-opened since this is blocked by bug 141757

[Alex Christensen]

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.

[Benjamin Poulain]

We'll use an interpreter instead.