Summary: | [Content Extensions] Add 3 byte jump size | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||
Component: | WebCore Misc. | Assignee: | Alex Christensen <achristensen> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | ||||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Alex Christensen
2015-06-29 14:05:31 PDT
Created attachment 255776 [details]
Patch
This reduced total byte code size in a large test from 14970936 bytes to 13207142 bytes. Created attachment 255777 [details]
Patch
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? Created attachment 255966 [details]
Patch
Created attachment 255981 [details]
Patch
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? (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. (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. 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. |