Bug 146425 – [Content Extensions] Add 3 byte jump size

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

Many jumps can be represented with 3 bytes, and adding an Int24 jump type significantly reduces byte code size.

[Alex Christensen]

Created attachment 255776 [details]
Patch

[Alex Christensen]

This reduced total byte code size in a large test from 14970936 bytes to 13207142 bytes.

[Alex Christensen]

Created attachment 255777 [details]
Patch

[Benjamin Poulain]

Comment on attachment 255777 [details]
Patch

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

> Source/WebCore/contentextensions/DFABytecode.h:96
> +    if (longestPossibleJump < 256 * 256 * 256 && longestPossibleJump >= -256 * 256 * 256)

It would prefer if "256 * 256 * 256 - 1" and "256 * 256 * 256" had names (Int24Min and Int24Max)

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:374
> +            RELEASE_ASSERT(distance >= -256 * 256 * 256 && distance < 256 * 256 * 256);

Ditto.

> Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:90
> +        return getBits<uint16_t>(bytecode, bytecodeLength, index, pagesUsed) & static_cast<int32_t>(getBits<int8_t>(bytecode, bytecodeLength, index + sizeof(uint16_t), pagesUsed)) << 16;

"Binary and" between the two parts?

[Alex Christensen]

Created attachment 255966 [details]
Patch

[Alex Christensen]

Created attachment 255981 [details]
Patch

[Darin Adler]

Comment on attachment 255981 [details]
Patch

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

I’m saying review+ but I think this change without testing is really risky.

> Source/WebCore/contentextensions/DFABytecode.h:90
> +const int32_t Int24Max = 256 * 256 * 256 - 1;
> +const int32_t Int24Min = -256 * 256 * 256;

I believe these are incorrect. It should be 128 * 256 * 256. And that means that we need to figure out some way to test this. It shouldn’t just be guesswork on my part!

I think these would be easier to read in hex like this:

    const int32_t Int24Max = 0x007FFFFFF;
    const int32_t Int24Max = 0xFF800000;

Or:

    const int32_t Int24Max = 0x007FFFFFF;
    const int32_t Int24Max = -0x00800000;

Or with shifting like this:

    const int32_t Int24Max = (1 << 23) - 1;
    const int32_t Int24Min = -(1 << 23);

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:375
> +            setBits<uint16_t>(m_bytecode, linkRecord.jumpLocation, static_cast<uint16_t>(distance));

Does this do the right thing for negative numbers?

[Alex Christensen]

(In reply to comment #7)
> Comment on attachment 255981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255981&action=review
> 
> I’m saying review+ but I think this change without testing is really risky.
I agree, and adding a test big enough to test large jumps like this would timeout while compiling.  I'll look into writing a test before landing.
> > Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:375
> > +            setBits<uint16_t>(m_bytecode, linkRecord.jumpLocation, static_cast<uint16_t>(distance));
> 
> Does this do the right thing for negative numbers?
Yes.  The sign is in the high bits, which I store as an int8_t.

[Alex Christensen]

(In reply to comment #7)
> Comment on attachment 255981 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255981&action=review
> 
> I’m saying review+ but I think this change without testing is really risky.
> 
> > Source/WebCore/contentextensions/DFABytecode.h:90
> > +const int32_t Int24Max = 256 * 256 * 256 - 1;
> > +const int32_t Int24Min = -256 * 256 * 256;
> 
> I believe these are incorrect. It should be 128 * 256 * 256. And that means
> that we need to figure out some way to test this. It shouldn’t just be
> guesswork on my part!
Those are indeed incorrect.

[Alex Christensen]

http://trac.webkit.org/changeset/186649

[Alex Christensen]

http://trac.webkit.org/changeset/186681 added some tests for this.  Those tests would fail if either the | were incorrect or the Int24Max were incorrect.