Bug 191062 – Align entries in metadata table

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

[Dominik Inführ]

Align entries in metadata table

[Dominik Inführ]

Created attachment 353361 [details]
Patch

[Dominik Inführ]

This is necessary for some 32-bit architectures that don't support most unaligned accesses.

[Tadeu Zagallo]

I'm not officially a reviewer, so I can't r+ the patch, but looks good to me. However, we probably only want to do that for the platforms that don't like the unaligned accesses.

[Dominik Inführ]

Thanks for taking a looking! I was also wondering about the same thing, in the end I aligned the metadata on all platforms since unaligned accesses are not free there either and just aligning shouldn't increase memory usage too much. What do you think?

[Filip Pizlo]

(In reply to Dominik Inführ from comment #4)
> Thanks for taking a looking! I was also wondering about the same thing, in
> the end I aligned the metadata on all platforms since unaligned accesses are
> not free there either and just aligning shouldn't increase memory usage too
> much. What do you think?

It should be optional. Unaligned accesses are effectively free on some platforms.

[Dominik Inführ]

Created attachment 353371 [details]
Patch

[Dominik Inführ]

Great, I made the alignment optional on CPU(NEEDS_ALIGNED_MEMORY).

[Yusuke Suzuki]

Comment on attachment 353371 [details]
Patch

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

> Source/JavaScriptCore/bytecode/Opcode.cpp:202
> +static unsigned metadataAlignments[] = {
> +
> +#define METADATA_ALIGNMENT(size) size,
> +    FOR_EACH_BYTECODE_METADATA_ALIGNMENT(METADATA_ALIGNMENT)
> +#undef METADATA_ALIGNMENT
> +
> +};

This data is not necessary if `CPU(NEEDS_ALIGNED_ACCESS)` is false. Let's guard with this condition and drop this data from the binary.

> Source/JavaScriptCore/bytecode/Opcode.cpp:212
> +unsigned metadataAlignment(OpcodeID opcodeID)
> +{
> +    return metadataAlignments[opcodeID];
> +}

Ditto.

[Keith Miller]

Comment on attachment 353371 [details]
Patch

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

> Source/JavaScriptCore/bytecode/MetadataTable.h:59
> +        for (; metadata + 1 <= end; ++metadata)

Can we make this metadata < end?

[Dominik Inführ]

Created attachment 353439 [details]
Patch

[Dominik Inführ]

I've updated the patch, metadataAlignment is now only included if aligned memory access is required.

@Keith: Not sure I can write this condition like this. Please correct me if I am wrong but wouldn't `metadata < end` run one iteration too often if there is an alignment gap at the end?

[Keith Miller]

(In reply to Dominik Inführ from comment #11)
> I've updated the patch, metadataAlignment is now only included if aligned
> memory access is required.
> 
> @Keith: Not sure I can write this condition like this. Please correct me if
> I am wrong but wouldn't `metadata < end` run one iteration too often if
> there is an alignment gap at the end?

Yeah, nvm ignore me.

[WebKit Commit Bot]

Comment on attachment 353439 [details]
Patch

Clearing flags on attachment: 353439

Committed r237638: <https://trac.webkit.org/changeset/237638>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45701324>