Bug 205592 – [JSC] StructureChain should hold vector of StructureID

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] StructureChain should hold vector of StructureID

[Yusuke Suzuki]

Created attachment 386406 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 386406 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1476
> +    bineq t2, [t3], .opPutByIdSlow
> +    addp 4, t3
> +    structureIDToStructureWithScratchAndTable(t2, t1, t0)
> +    loadq Structure::m_prototype[t2], t2
>      bqneq t2, ValueNull, .opPutByIdTransitionChainLoop

We need to load Structure* from StructureID.
If it really matters, I think we should reconsider using StructureID: we should use Structure* and we should put it in 32GB region so that we can use 32bit pointer for that.
But, I think this does not matter much.

[Mark Lam]

Comment on attachment 386406 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/Structure.cpp:1217
> +    StructureID* structureIDVector = structureChain->head();

nit: I think "structureIDPointer" is a better name than "structureIDVector" here since we're using it as a bump pointer and not a vector.  Alternatively, "currentStructureID" or "structureIDIterator" also works.

> Source/JavaScriptCore/runtime/StructureChain.cpp:67
> +        vm.heap.writeBarrier(this);

Now that we're storing structureIDs and not setting a list of WriteBarrier<Structure>, I think we can put this writeBarrier() call after the loop.  There's nothing gained by calling it repeatedly in the loop.

[Yusuke Suzuki]

Comment on attachment 386406 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Structure.cpp:1217
>> +    StructureID* structureIDVector = structureChain->head();
> 
> nit: I think "structureIDPointer" is a better name than "structureIDVector" here since we're using it as a bump pointer and not a vector.  Alternatively, "currentStructureID" or "structureIDIterator" also works.

currentStructureID sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/StructureChain.cpp:67
>> +        vm.heap.writeBarrier(this);
> 
> Now that we're storing structureIDs and not setting a list of WriteBarrier<Structure>, I think we can put this writeBarrier() call after the loop.  There's nothing gained by calling it repeatedly in the loop.

Currently, storedPrototypeObject does not cause any GC related operations. But I think this is not guaranteed. So, make this safe, I like putting write-barrier here.

[Yusuke Suzuki]

Committed r253931: <https://trac.webkit.org/changeset/253931>

[Radar WebKit Bug Importer]

<rdar://problem/58228521>

[Saam Barati]

Comment on attachment 386406 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/StructureChain.cpp:67
>>> +        vm.heap.writeBarrier(this);
>> 
>> Now that we're storing structureIDs and not setting a list of WriteBarrier<Structure>, I think we can put this writeBarrier() call after the loop.  There's nothing gained by calling it repeatedly in the loop.
> 
> Currently, storedPrototypeObject does not cause any GC related operations. But I think this is not guaranteed. So, make this safe, I like putting write-barrier here.

But if you moved writeBarrier outside the loop, |this| is alive, and would be visited regardless of what is inside the loop. The loop could GC all it wants and we should be safe. So Mark is correct that it could move out to the end of the loop (and it could’ve in the old code too). (That said, I’m not sure it matters much in practice.)

[Yusuke Suzuki]

Comment on attachment 386406 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/StructureChain.cpp:67
>>>> +        vm.heap.writeBarrier(this);
>>> 
>>> Now that we're storing structureIDs and not setting a list of WriteBarrier<Structure>, I think we can put this writeBarrier() call after the loop.  There's nothing gained by calling it repeatedly in the loop.
>> 
>> Currently, storedPrototypeObject does not cause any GC related operations. But I think this is not guaranteed. So, make this safe, I like putting write-barrier here.
> 
> But if you moved writeBarrier outside the loop, |this| is alive, and would be visited regardless of what is inside the loop. The loop could GC all it wants and we should be safe. So Mark is correct that it could move out to the end of the loop (and it could’ve in the old code too). (That said, I’m not sure it matters much in practice.)

Is it correct? Let's consider the following case,

Now, we attempt to store 3rd StructureID. We already did GC when creating 1st StructureID. And, after that, concurrent marking happens before 2nd StructureID is stored.
2nd Structure is not in newly-allocated space. The status is,

1. |this| is already marked, but this marking only visits 1st StructureID.
2. 2nd StructureID is already stored to the vector

Then, GC happens in storedPrototypeObject. In this case, 2nd Structure can be missed because of lack of write-barrier.