Bug 68717 – Make write barriers actually do something when enabled

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

[Oliver Hunt]

Make write barriers actually do something when enabled

[Oliver Hunt]

Created attachment 108512 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1061:  movb_i8m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1068:  movb_i8m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 17 files


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

[Oliver Hunt]

Created attachment 108535 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1061:  movb_i8m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1068:  movb_i8m is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 17 files


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

[Geoffrey Garen]

r=me with those changes and a working build.

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1245
> +    OwnPtr<GPRTemporary> temp1;
> +    OwnPtr<GPRTemporary> temp2;
> +    if (scratch1 == InvalidGPRReg) {
> +        temp1 = adoptPtr(new GPRTemporary(this));
> +        scratch1 = temp1->gpr();
> +    }
> +    if (scratch2 == InvalidGPRReg) {
> +        temp2 = adoptPtr(new GPRTemporary(this));
> +        scratch2 = temp2->gpr();
> +    }

Using OwnPtr here is a bit awkward and inefficient. I think this would be better:
* Give GPRTemporary a default constructor that initializes m_jit to NULL and m_gpr to InvalidGPRReg.
* Change GPRTemporary's destructor to return early if m_jit is NULL.
* Give GPRTemporary an adopt function that takes "GPRTemporary& other" as an argument, steals other's data, and null out other.

Then:
 1236    GPRTemporary temp1;
 1237    GPRTemporary temp2;
 1238    if (scratch1 == InvalidGPRReg) {
 1239        temp1.adopt(GPRTemporary(this));
 1240        scratch1 = temp1->gpr();
 1241    }
 1242    if (scratch2 == InvalidGPRReg) {
 1243        temp2.adopt(GPRTemporary(this));
 1244        scratch2 = temp2->gpr();
 1245    }

> Source/JavaScriptCore/dfg/DFGJITCodeGenerator.cpp:1279
> +    OwnPtr<GPRTemporary> temp;
> +    if (scratch == InvalidGPRReg) {
> +        temp = adoptPtr(new GPRTemporary(this));
> +        scratch = temp->gpr();
> +    }

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:27
> +

No space here please.

> Source/JavaScriptCore/heap/CardSet.h:44
> +    bool cardMarkedForAtom(const void*);

"isCardMarkedForAtom", please, since this is a boolean test and not a notification.

> Source/JavaScriptCore/heap/CardSet.h:45
> +    void markCardForAtom(const void*);

Soon you will need clearCardForAtom or clearAll, too, no?

> Source/JavaScriptCore/heap/Heap.h:248
> +        MarkedBlock::blockFor(owner)->setDirtyObject(owner);

Let's just call this "writeBarrier" too.

[Oliver Hunt]

Committed r95865: <http://trac.webkit.org/changeset/95865>