1163168 - Prettify IonAssemblerBuffer.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 0001-Prettify-IonAssemblerBuffer.h.patch

This long-overdue patch updates IonAssemblerBuffer.h to fit SM style, have clearer variable and function names, and have a human-readable implementation of getInst().

Comment 1

[Douglas Crosher [:dougc]]

Comment on attachment 8603574 [details] [diff] [review]
0001-Prettify-IonAssemblerBuffer.h.patch

Review of attachment 8603574 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok. Some comments fwiw.

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +104,5 @@
> +
> +  protected:
> +    // Doubly-linked list of BufferSlices, with the most recent in tail position.
> +    Slice* firstSlice;
> +    Slice* lastSlice;

'head' and 'tail' seemed fine, and are commonly used for lists.

@@ +160,5 @@
> +        if (slice == nullptr)
> +            return fail_oom();
> +
> +        // If this is the first Slice in the buffer, add to head position.
> +        if (MOZ_UNLIKELY(!firstSlice)) {

Not sure if this 'MOZ_UNLIKELY' and others below are really needed here. It's probably not hot code, and there might be lots of small code objects.

@@ +168,3 @@
>          }
> +
> +        // Add the new slice to the linked list of slices.

This block is finishing the current slice, adding its size to the running total, and linking the current slice to the new slice.

@@ +286,5 @@
> +        if (offset >= int(bufferSize))
> +            return (Inst*)&lastSlice->instructions[offset - bufferSize];
> +
> +        // How close is this offset to the previous one we looked up?
> +        // If it is sufficiently far from the start and back of the buffer,

'start and end'

@@ +288,5 @@
> +
> +        // How close is this offset to the previous one we looked up?
> +        // If it is sufficiently far from the start and back of the buffer,
> +        // use the finger to start midway through the list.
> +        int finger_dist = abs((int)(offset - finger_offset));

Lots of sign coercions here. Does the BufferOffset need to be signed and could it be changed to unsigned? Or could finger_offset is changed to be signed?

Comment 2

[Sean Stangl [:sstangl]]

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/913091cadf63

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/913091cadf63