Bug 194419 – [JSC] Shrink sizeof(CodeBlock) more

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Shrink sizeof(CodeBlock) more

[Yusuke Suzuki]

Created attachment 361469 [details]
Patch

[EWS Watchlist]

Attachment 361469 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:94:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 361477 [details]
Patch

[EWS Watchlist]

Attachment 361477 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:94:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 361482 [details]
Patch

[EWS Watchlist]

Attachment 361482 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:94:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzing  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 361482 [details]
Patch

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

Very nice.  r=me with suggestions.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:526
> -    for (const auto& instruction : *m_instructions) {
> +    for (const auto& instruction : instructions()) {

Pre-cache instructions() in a local before the loop so that the compiler knows that it won't change on each iteration.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1074
> +            auto instruction = instructions().at(propertyAccessInstructions[i]);

Precache instructions in a local before loop.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1204
> +        const auto curInstruction = instructions().at(propertyAccessInstructions[i]);

Precache instructions in a local before loop.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1711
> +    auto instruction = instructions().at(bytecodeOffset);

Use auto& to make is explicit at a glance that this is a reference.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1816
> +    for (const auto& it : instructions()) {

Precache instructions in a local before loop.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2755
> +    for (const auto& instruction : instructions()) {

Precache instructions in a local before loop.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2944
> +    for (const auto& instruction : instructions()) {

Precache instructions in a local before loop.

[Yusuke Suzuki]

Comment on attachment 361482 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:526
>> +    for (const auto& instruction : instructions()) {
> 
> Pre-cache instructions() in a local before the loop so that the compiler knows that it won't change on each iteration.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1074
>> +            auto instruction = instructions().at(propertyAccessInstructions[i]);
> 
> Precache instructions in a local before loop.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1204
>> +        const auto curInstruction = instructions().at(propertyAccessInstructions[i]);
> 
> Precache instructions in a local before loop.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1711
>> +    auto instruction = instructions().at(bytecodeOffset);
> 
> Use auto& to make is explicit at a glance that this is a reference.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1816
>> +    for (const auto& it : instructions()) {
> 
> Precache instructions in a local before loop.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2755
>> +    for (const auto& instruction : instructions()) {
> 
> Precache instructions in a local before loop.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2944
>> +    for (const auto& instruction : instructions()) {
> 
> Precache instructions in a local before loop.

Fixed.

[Yusuke Suzuki]

Committed r241222: <https://trac.webkit.org/changeset/241222>

[Radar WebKit Bug Importer]

<rdar://problem/47934328>

[Darin Adler]

Comment on attachment 361482 [details]
Patch

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

>>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:526
>>> +    for (const auto& instruction : instructions()) {
>> 
>> Pre-cache instructions() in a local before the loop so that the compiler knows that it won't change on each iteration.
> 
> Fixed.

While it’s OK to do this if you prefer it from a stylistic point of view, there is no need to change it for correct behavior. The way a range for loop is defined, the compiler will not and must not call instructions() more than once. An example of an on-line reference that makes this clear is <https://en.cppreference.com/w/cpp/language/range-for>.