Bug 142759 - Compile character ranges targeting the same state as range check in the bytecode
Summary: Compile character ranges targeting the same state as range check in the bytecode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-16 18:10 PDT by Benjamin Poulain
Modified: 2015-03-17 13:48 PDT (History)
1 user (show)

See Also:


Attachments
Patch (18.29 KB, patch)
2015-03-16 18:18 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (18.24 KB, patch)
2015-03-16 20:24 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (20.32 KB, patch)
2015-03-16 20:39 PDT, Benjamin Poulain
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-03-16 18:10:17 PDT
Compile character ranges targeting the same state as range check in the bytecode
Comment 1 Benjamin Poulain 2015-03-16 18:18:33 PDT
Created attachment 248774 [details]
Patch
Comment 2 Alex Christensen 2015-03-16 19:02:55 PDT
Comment on attachment 248774 [details]
Patch

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

LGTM.  r- because there are a few small things to address.

> Source/WebCore/contentextensions/DFABytecode.h:47
> +    // The index to jump to if the values are equal (4 bytes).

"if the value is in the range"

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:117
> +    uint16_t lowValueKey;

I think either this should be a uint8_t or compileCheckForRange should take a uint16_t.  It's not easy to see where the type changes.
Also, I think rangeMin would be a better name for this variable.

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:118
> +    unsigned lowValueDestination = 0;

lowValueDestination -> rangeDestination?

> Source/WebCore/contentextensions/NFAToDFA.cpp:344
> +        if (!dfaNode.hasFallbackTransition && dfaNode.transitions.size() >= 126) {

This code is quite generalized.  I think it could be simplified:
If size==127, then everything goes to the fallback transition.  
If size==126, then we only have to find the one transition that is missing.

> Source/WebCore/contentextensions/NFAToDFA.cpp:362
> +            ASSERT_WITH_MESSAGE(bestTargetScore, "There should be a list one valid target since having transitions is a precondition to enter this path.");

"a list" -> "at least"
They sound the same with a Belgian accent :)
Comment 3 Benjamin Poulain 2015-03-16 20:24:28 PDT
Created attachment 248793 [details]
Patch
Comment 4 Benjamin Poulain 2015-03-16 20:39:44 PDT
Created attachment 248797 [details]
Patch
Comment 5 Alex Christensen 2015-03-17 10:32:07 PDT
Comment on attachment 248797 [details]
Patch

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

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:116
> +    bool hasrangeMin = false;

hasRangeMin

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:156
> +void DFABytecodeCompiler::compileCheckForRange(uint16_t lowValue, uint16_t highValue, unsigned destinationNodeIndex)

I think it would be good to assert lowValue and highValue are less than 128 here because we cast them to 8-bit integers.

> Source/WebCore/contentextensions/NFAToDFA.cpp:346
> +            && ((dfaNode.transitions.size() == 126 && !dfaNode.transitions.contains(0))
> +                || (dfaNode.transitions.size() == 127 && dfaNode.transitions.contains(0)))) {

I'm not sure I understand why the null character is a special case here.  Isn't it just another transition?
Comment 6 Benjamin Poulain 2015-03-17 13:48:15 PDT
Committed r181663: <http://trac.webkit.org/changeset/181663>