Bug 180084 – [JSC] Use JSFixedArray for op_new_array_buffer

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

[Yusuke Suzuki]

[JSC] Use JSFixedArray for op_new_array_buffer

[Yusuke Suzuki]

Created attachment 327754 [details]
Patch

[Yusuke Suzuki]

Created attachment 327757 [details]
Patch

[Saam Barati]

Comment on attachment 327757 [details]
Patch

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

r=me. This is a nice cleanup!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3958
> +                m_jit.store32(Imm32(u.halves[0]), MacroAssembler::Address(storageGPR, sizeof(JSValue) * index));
> +                m_jit.store32(Imm32(u.halves[1]), MacroAssembler::Address(storageGPR, sizeof(JSValue) * index + sizeof(int32_t)));

I know this is old code you're modifying, but it feels outdated. Why not just use EncodedJSValue, and do this in terms of JSValue::tag()/JSValue::payload() and their offsets?

> Source/JavaScriptCore/runtime/JSFixedArray.h:65
> +    static JSFixedArray* create(VM& vm, unsigned length)
> +    {
> +        auto* array = tryCreate(vm, vm.fixedArrayStructure.get(), length);
> +        RELEASE_ASSERT(array);
> +        return array;
> +    }

It's unfortunate that this might make the bytecode generator crash. Maybe you can use tryCreate, and if it fails, you can just emit throwing an OOM exception in bytecode? Or if the bytecode generator itself has a way of throwing an exception?

Maybe it's not worth it though. I'll let you decide if you think we should do it. Or we can make it a FIXME

[Yusuke Suzuki]

Comment on attachment 327757 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSFixedArray.h:65
>> +    }
> 
> It's unfortunate that this might make the bytecode generator crash. Maybe you can use tryCreate, and if it fails, you can just emit throwing an OOM exception in bytecode? Or if the bytecode generator itself has a way of throwing an exception?
> 
> Maybe it's not worth it though. I'll let you decide if you think we should do it. Or we can make it a FIXME

Currently we do not have consistent mechanism to throw an OOM error for bytecode generation.
I think we should have this. But I think this should be done in a separate patch.

[Yusuke Suzuki]

Comment on attachment 327757 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3958
>> +                m_jit.store32(Imm32(u.halves[1]), MacroAssembler::Address(storageGPR, sizeof(JSValue) * index + sizeof(int32_t)));
> 
> I know this is old code you're modifying, but it feels outdated. Why not just use EncodedJSValue, and do this in terms of JSValue::tag()/JSValue::payload() and their offsets?

In the case of ArrayWithDouble, we would like to store double value (as int64_t) instead of EncodedJSValue. That's why we use this style.

[Yusuke Suzuki]

Committed r225385: <https://trac.webkit.org/changeset/225385>

[Radar WebKit Bug Importer]

<rdar://problem/35792563>