Closed Bug 1163168 Opened 5 years ago Closed 5 years ago

Prettify IonAssemblerBuffer.h

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: sstangl, Assigned: sstangl)

References

Details

Attachments

(1 file)

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().
Attachment #8603574 - Flags: review?(dtc-moz)
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?
Attachment #8603574 - Flags: review?(dtc-moz) → review+
https://hg.mozilla.org/mozilla-central/rev/913091cadf63
Assignee: nobody → sstangl
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.