[webkit-reviews] review granted: [Bug 180084] [JSC] Use JSFixedArray for op_new_array_buffer : [Attachment 327757] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 180084: [JSC] Use JSFixedArray for op_new_array_buffer
https://bugs.webkit.org/show_bug.cgi?id=180084

Attachment 327757: Patch

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




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 327757
  --> https://bugs.webkit.org/attachment.cgi?id=327757
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